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.

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

Fix expectations of JMX test #5764

Merged
merged 2 commits into from
Jan 7, 2025
Merged

Fix expectations of JMX test #5764

merged 2 commits into from
Jan 7, 2025

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jan 7, 2025

No description provided.

@atoulme atoulme requested review from a team as code owners January 7, 2025 00:23
@crobert-1
Copy link
Contributor

Can you provide more context behind why the existing test is incorrect? It looks like some units changed, more metrics are being sent now, and potentially other changes have happened upstream.

I'm clarifying to ensure these changes are expected and documented, not something that's going to break something for users.

@atoulme
Copy link
Contributor Author

atoulme commented Jan 7, 2025

The JMX receiver itself updated, and the test fails because of the jmx upgrade of the jmx-metrics-gatherer jar: #5729

We unfortunately don't run all integration tests on it or we would have caught it then.

The jmx-metrics-gatherer jar is cut by the opentelemetry-java-project, and they have this item in their release notes:
https://github.com/open-telemetry/opentelemetry-java-contrib/releases/tag/v1.42.0

open-telemetry/opentelemetry-java-contrib#1591

It looked mostly that we updated units, and nothing else. What new metrics are we adding? That might be a compounding effect of missing updates for this jar for a version or two. I will review the diff.

@atoulme
Copy link
Contributor Author

atoulme commented Jan 7, 2025

I reviewed, I don't see new metrics. Some metrics were reordered, most likely as golden changed the way it writes yaml to order keys or the order of the metrics was different. I think we're OK.

@jinja2
Copy link
Contributor

jinja2 commented Jan 7, 2025

We do run these tests on upgrades to jar version. The issue seems to be the order in which it was merged. This jar version is introduced in 0.116 of the collector and we merged the jar before jmxreceiver was updated to 0.116. The jar update MR had the tests failing due to the version not being supported in the receiver yet.

@jinja2 jinja2 merged commit e43c916 into main Jan 7, 2025
64 checks passed
@jinja2 jinja2 deleted the fix_jmx_expectations branch January 7, 2025 18:31
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2025
@crobert-1
Copy link
Contributor

Thanks for clarifying @atoulme, appreciate the information and clarification, LGTM 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants