Skip to content

Conversation

@breedx-splk
Copy link
Contributor

Tomcat integration test breaks when updating to the new instrumentation. This fixes it. Would love to have a look from @SylvainJuge and @robsunday. Thanks!

@breedx-splk breedx-splk requested a review from a team July 2, 2025 20:22
@breedx-splk
Copy link
Contributor Author

Note: This is PRing into the renovate branch in #1983.

@otelbot-java-contrib
Copy link
Contributor

🔧 The result from spotlessApply was committed to the PR branch.

@SylvainJuge
Copy link
Contributor

Those integration tests test the yaml definitions that are part of the jmx-scraper for the "legacy" definitions (the ones that are close to jmx-gatherer), and we need to preserve that for a bit of time to avoid regressions, even if those definitions are quite unlikely to change.

By default, the otel.jmx.target.source is set to auto, which means that the definitions from instrumentation have priority when available, which is the case for tomcat here.

This already already happened when the JVM metrics were added in instrumentation, and the fix was to set otel.jmx.target.source to legacy (here) to make sure that we continue to test the "legacy" yaml metrics definitions.

Instead of modifying tomcat tests to test the instrumentation definitions (which are already tested in instrumentation), I would suggest to do the following:

  • revert the changes done to tomcat integration tests
  • set otel.jmx.target.source to legacy for all those integration tests, because they should focus on the legacy definitions which are not tested otherwise, the best place to do that would be in TargetSystemIntegrationTest#startContainers
  • because it's now applied to all integration tests by default, remove the similar modification in JvmIntegrationTest#customizeScraperContainer.

With that, it should ensure that we don't get test failures for the next instrumentation dependency upgrades.

I don't think it is necessary to test the inheritance from instrumentation as this should be covered with JmxScraperConfigTest#targetSystemSource_auto test, however it's not really testing inheritance from an actual yaml file but one that happens to be in the expected location in classpath.

@breedx-splk
Copy link
Contributor Author

Thanks @SylvainJuge for the detailed explanation. I'll roll those test changes back and see how it looks with otel.jmx.target.source=legacy. Appreciate it. 🙏🏻

@breedx-splk breedx-splk force-pushed the fix_tomcat_integration_test branch from 07dad29 to bd26277 Compare July 3, 2025 16:00
Copy link
Contributor

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

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

LGTM, I'll open another pr to implement a more generic approach later.

@breedx-splk
Copy link
Contributor Author

  • TargetSystemIntegrationTest

Thanks, I probably pushed prematurely. I am already building the "legacy" into TargetIntegrationSystemTest but got pulled into other stuff. I'll push here momentarily. Looks good!

@github-actions github-actions bot requested review from SylvainJuge and robsunday July 3, 2025 17:33
@breedx-splk breedx-splk merged commit 92c279c into open-telemetry:renovate/otelinstrumentationversion Jul 3, 2025
48 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants