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

Cover changes in 3.15.4 backports #2318

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Cover changes in 3.15.4 backports #2318

wants to merge 2 commits into from

Conversation

fedinskiy
Copy link
Contributor

@fedinskiy fedinskiy commented Feb 27, 2025 •
edited
Loading

Summary

  • Add checks for exemplars in metrics (Covers QUARKUS-5424)
  • Verify, that dynamic tag is not used in netty metrics (QUARKUS-5637)

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@fedinskiy fedinskiy force-pushed the feature/exemplar-metrics branch from 2fa45ff to de2d41a Compare February 27, 2025 10:42
michalvavrik
michalvavrik reviewed Feb 27, 2025
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

I think this test is alright, but if there is a TP, let's link it so that I know what to verify.

@rsvoboda
Copy link
Member

There is no TP

@rsvoboda
Copy link
Member

Out of curiosity, are exemplars available in OpenTelemetry metrics?

@fedinskiy
Copy link
Contributor Author

@rsvoboda do you mean this: https://github.com/quarkiverse/quarkus-micrometer-registry/blob/main/micrometer-registry-otlp/pom.xml ?
I didn't check, since it is in quarkiverse, but I can take a look.

The exemplars are definitely not available without quarkus-opentelemetry extensions (I tried to run the same check in micrometer-prometheus module).

michalvavrik
michalvavrik approved these changes Feb 27, 2025
View reviewed changes
Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

Ok, if no TP is expected, I think this tests what it says and tests are passing, code looks good

@fedinskiy fedinskiy force-pushed the feature/exemplar-metrics branch 2 times, most recently from f0ccfae to 71db754 Compare February 27, 2025 12:43
@fedinskiy fedinskiy force-pushed the feature/exemplar-metrics branch from 71db754 to baed534 Compare February 27, 2025 12:50
@fedinskiy fedinskiy changed the title Add checks for exemplars in metrics Cover changes in 3.15.4 backports Feb 27, 2025
@rsvoboda
Copy link
Member

@michalvavrik @fedinskiy I meant https://quarkus.io/guides/opentelemetry-metrics

The doc is talking about exemplars in the example, so hopefully it works.

@rsvoboda
Copy link
Member

PR title could be more specific

michalvavrik
michalvavrik reviewed Feb 27, 2025
View reviewed changes
...ing/opentelemetry-reactive/src/test/java/io/quarkus/ts/opentelemetry/reactive/MetricsIT.java

@Test
@Tag("QUARKUS-5637")
public void testNetty() {
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT this test verifies that metrics doesn't have high cardinality label as id because has too many values, I'd probably call it differently, something like testNettyIdLabelNotPresent or whatever suits you

@michalvavrik
Copy link
Member

Still looks good even with addion and changes. I am not going to approve again while there is don't merge label. Can't comment on the title as I am not sure what will be final state of this PR (if there is more feature, it can be hard to name...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalvavrik michalvavrik michalvavrik approved these changes

Assignees
No one assigned
Labels
DON'T MERGE triage/backport-3.15
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fedinskiy @rsvoboda @michalvavrik