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

Add missing metrics for Jersey in cases of client errors #4326

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

adpaste
Copy link
Contributor

@adpaste adpaste commented Nov 9, 2023

We are using Jersey integration and we have noticed that some 40x errors (such as 406 Not Acceptable or 415 Unsupported Media Type) are not reported in the metrics.

After some debugging, we found out that the root cause of this issue is that only one kind of client exception (NotFoundException) is taken into account during metric collection.

This pull request should fix the problem, by taking into account all types of ClientErrorExceptions.

@pivotal-cla
Copy link

@adpaste Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@adpaste Thank you for signing the Contributor License Agreement!

@adpaste adpaste marked this pull request as ready for review November 9, 2023 13:41
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thanks for reporting the issue and providing a fix along with a test. Could you take a look at the one comment I had and rebase the pull request on 1.9.x so we can accept it as a bug fix? Also, note that the Jersey instrumentation was added to a jersey-micrometer module in the Jersey repo (see eclipse-ee4j/jersey#5391). We plan to deprecate the code in the Micrometer repo in favor of that (see #4100). So please send a pull request over there too if you have the time and consider migrating to using that over what will be deprecated here.

Thanks again!

@shakuzen shakuzen added this to the 1.9.x milestone Dec 21, 2023
@shakuzen shakuzen added bug A general bug module: micrometer-core An issue that is related to our core module labels Dec 21, 2023
@adpaste adpaste force-pushed the jersey-client-errors branch from 20ac7d6 to c4ed2dd Compare January 3, 2024 09:09
@adpaste adpaste changed the base branch from main to 1.9.x January 3, 2024 09:10
@adpaste
Copy link
Contributor Author

adpaste commented Jan 3, 2024

Thanks for the feedback. I replied to the comment, and rebased the PR to 1.9.x.
I'll look into migrating to jersey-micrometer.

@adpaste adpaste force-pushed the jersey-client-errors branch from 66bffd1 to 5cdf589 Compare March 1, 2024 12:17
@shakuzen shakuzen modified the milestones: 1.9.x, 1.9.18 Mar 4, 2024
@shakuzen shakuzen merged commit 8378a58 into micrometer-metrics:1.9.x Mar 4, 2024
7 checks passed
@adpaste adpaste deleted the jersey-client-errors branch March 4, 2024 09:05
@shakuzen shakuzen added the instrumentation An issue that is related to instrumenting a component label Mar 4, 2024
@shakuzen
Copy link
Member

shakuzen commented Mar 4, 2024

It looks like this broke tests in the 1.11.x branch. I'm looking into it now.
https://app.circleci.com/pipelines/github/micrometer-metrics/micrometer/6107/workflows/6c026dde-bd49-4bb1-a8d7-bdb0f07168b3/jobs/30900

shakuzen added a commit that referenced this pull request Mar 4, 2024
This applies the changes from gh-4326 that were made to JerseyTags to JerseyKeyValues.
@shakuzen
Copy link
Member

shakuzen commented Mar 4, 2024

I was able to fix it by applying similar changes to JerseyKeyValues (which is a copy of JerseyTags for Observation-based instrumentation). See 66c1d47

adpaste added a commit to adpaste/jersey that referenced this pull request Apr 4, 2024
adpaste added a commit to adpaste/jersey that referenced this pull request Apr 4, 2024
adpaste added a commit to adpaste/jersey that referenced this pull request Apr 5, 2024
senivam pushed a commit to eclipse-ee4j/jersey that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants