Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add stop-sending option #474

Merged
merged 6 commits into from
Dec 18, 2024
Merged

add stop-sending option #474

merged 6 commits into from
Dec 18, 2024

Conversation

jackshirazi
Copy link
Contributor

No description provided.

@jackshirazi jackshirazi changed the title add stop-sending option with tests add stop-sending option Dec 6, 2024
@jackshirazi jackshirazi marked this pull request as ready for review December 10, 2024 12:59
@jackshirazi jackshirazi requested a review from a team December 10, 2024 13:00
JonasKunz
JonasKunz reviewed Dec 11, 2024
View reviewed changes
custom/src/main/java/co/elastic/otel/config/DynamicConfiguration.java Outdated
private Boolean recoverySendMetricsState;

private boolean initSendingStates() {
if (recoverySendSpansState == null && ElasticSpanExporter.getInstance() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log warning if the exporter instances are null here? This means that the setting might not be correctly respected initially, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good call

smoke-tests/src/test/java/com/example/javaagent/smoketest/DynamicConfigSmokeTest.java
@AfterEach
public void endTest() throws InterruptedException {
doRequest(getUrl("/dynamicconfig/reset"), okResponseBody("reset"));
Thread.sleep(500L); // give the reset time to be applied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for using the smoke-tests instead of an integration-test ?
I prefer the integration tests because they are easier to follow, debug and maintain and still run the full agent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was a reason which is that I prefer the smoke tests and find the integration tests non-transparent in how they work. Though I take your point they are easier to maintain. And these tests are actually integration testst. I've done an initial pass and found a bug the integration test identified that the smoke tests didn't, so I'll map them over

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per discussion (the integration test is unable to handle the exporter wrapping correctly because it has a custom exporter that doesn't go through configuration), staying with the smoke test

@jackshirazi jackshirazi merged commit dbc12ca into elastic:main Dec 18, 2024
17 checks passed
@jackshirazi jackshirazi mentioned this pull request Dec 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LikeTheSalad LikeTheSalad LikeTheSalad approved these changes

@JonasKunz JonasKunz JonasKunz approved these changes

Assignees
No one assigned
Labels
agent-java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jackshirazi @JonasKunz @LikeTheSalad