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

More accurate metrics for HTTP redirect responses from Jersey #3332

Merged
merged 3 commits into from Aug 10, 2022

Conversation

tinolazreg
Copy link
Contributor

Fixes #3327

@pivotal-cla
Copy link

@tinolazreg Please sign the Contributor License Agreement!

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

See the FAQ for frequently asked questions.

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! It looks good to me. Could we add a test for the case when there is no matched route?
Also, we will need the CLA signed to accept the contribution.

@pivotal-cla
Copy link

@tinolazreg Thank you for signing the Contributor License Agreement!

@tinolazreg
Copy link
Contributor Author

Thanks! It looks good to me. Could we add a test for the case when there is no matched route? Also, we will need the CLA signed to accept the contribution.

Great! I have signed the CLA now. I tried creating a case for the test that you mentioned, but found it difficult to create a similar test with a HTTP redirection without a matching route, that ends up in the JerseyTags.

@shakuzen shakuzen changed the base branch from main to 1.8.x August 10, 2022 04:07
@shakuzen
Copy link
Member

I tried creating a case for the test that you mentioned, but found it difficult to create a similar test with a HTTP redirection without a matching route, that ends up in the JerseyTags.

Thank you for trying. I tried a bit myself and also struggled to come up with something. I don't like putting something in that isn't tested, but the logic makes sense and is similar to instrumentation we have elsewhere.

I've rebased the pull request on the 1.8.x branch so we can get this fixed in upcoming maintenance releases. The more I've thought about it, the more the previous behavior was more bug-like. Once merged, snapshots will be available if you would like to test things out with your application before the release.

Thank you for reporting and fixing this.

@shakuzen shakuzen merged commit f5f3e01 into micrometer-metrics:1.8.x Aug 10, 2022
@tinolazreg tinolazreg deleted the jerseyTagsRedirect branch August 11, 2022 11:28
@tinolazreg
Copy link
Contributor Author

I tried creating a case for the test that you mentioned, but found it difficult to create a similar test with a HTTP redirection without a matching route, that ends up in the JerseyTags.

Thank you for trying. I tried a bit myself and also struggled to come up with something. I don't like putting something in that isn't tested, but the logic makes sense and is similar to instrumentation we have elsewhere.

I've rebased the pull request on the 1.8.x branch so we can get this fixed in upcoming maintenance releases. The more I've thought about it, the more the previous behavior was more bug-like. Once merged, snapshots will be available if you would like to test things out with your application before the release.

Thank you for reporting and fixing this.

Thank you! Glad to be able to contribute.

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.

Uri tag replaced with REDIRECTION on all HTTP redirect responses with Jersey server
3 participants