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

Add response code meters for ResponseMetered annotation #3043

Merged
merged 7 commits into from
Jan 11, 2023
Merged

Add response code meters for ResponseMetered annotation #3043

merged 7 commits into from
Jan 11, 2023

Conversation

dennyac
Copy link
Contributor

@dennyac dennyac commented Dec 19, 2022 •
edited
Loading

Currently the ResponseMetered annotation gives us top level 1xx/2xx/3xx/4xx/5xx meters. We have many use cases where we care about specific response codes like 401/503 and end up manually instrumenting code to create these meters. This PR adds meters for individual response codes in addition to the 1xx/2xx/3xx/4xx/5xx meters in the ResponseMetered annotation.

Hoping others find this useful and I'm open to suggestions/feedback.

Happy to add this functionality to metrics-jersey3 and metrics-jersey31 as well if this approach is acceptable. Can also create a PR to add individual response code meters to the jetty InstrumentedHandler

@dennyac dennyac requested review from a team as code owners December 19, 2022 01:28
@gitpod-io
Copy link

gitpod-io bot commented Dec 19, 2022

joschi
joschi requested changes Dec 19, 2022
View reviewed changes
Copy link
Member

@joschi joschi left a comment

Choose a reason for hiding this comment

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

@dennyac Thanks for your contribution! It looks good in general.

Could you please make this change backward-compatible and optional by adding a flag to the ResponseMetered annotation which controls whether the detailed, the "coarse" (2xx, etc.), or both styles status codes are recorded?

Maybe an enum would make sense here: COARSE (default), DETAILED, and ALL (because BOTH might not be true anymore if another mode was added 😅).

@joschi joschi added the feature label Dec 19, 2022
@dennyac
Copy link
Contributor Author

dennyac commented Dec 20, 2022

@joschi - That makes sense. I'll incorporate that feedback. Thanks!

@dennyac dennyac force-pushed the add-response-code-meters-for-response-metered branch from b53546a to a945082 Compare December 22, 2022 02:02
@dennyac
Copy link
Contributor Author

dennyac commented Dec 22, 2022

@joschi - I've pushed changes based on your feedback. Please review when you get time.

Also, let me know if I should add this functionality to metrics-jersey3 and metrics-jersey31 modules as well (in a separate PR or the same PR)

@dennyac dennyac requested a review from joschi December 22, 2022 16:57
@dennyac
Copy link
Contributor Author

dennyac commented Jan 4, 2023

Happy New Year @joschi !!

Bumping this PR for review when you have time.

@dennyac
Copy link
Contributor Author

dennyac commented Jan 11, 2023

Thanks for the review @joschi !! Addressed your comments.

@dennyac dennyac requested a review from joschi January 11, 2023 17:33
@joschi joschi added this to the 4.2.16 milestone Jan 11, 2023
@joschi joschi enabled auto-merge (squash) January 11, 2023 21:10
@joschi
Copy link
Member

joschi commented Jan 11, 2023

@dennyac Thanks a lot for your contribution and the changes you've made to this PR! 😄

Happy to add this functionality to metrics-jersey3 and metrics-jersey31 as well if this approach is acceptable. Can also create a PR to add individual response code meters to the jetty InstrumentedHandler

If you want to invest the time into creating PRs for these I'll happily review and merge them. 😉

@joschi joschi merged commit 7360d7e into dropwizard:release/4.2.x Jan 11, 2023
@dennyac
Copy link
Contributor Author

dennyac commented Jan 11, 2023

@joschi - Thanks for taking the time to review the PRs. I'll create PRs for metrics-jersey3 and metrics-jersey31

This was referenced Jan 13, 2023
Merged
Merged
@dennyac dennyac mentioned this pull request Jan 30, 2023
Merged
joschi pushed a commit that referenced this pull request Feb 7, 2023
@dennyac
Add ResponseMeteredLevel parameter to ResponseMetered for Jersey 3.1.x (
a97b0bb 
#3116)

Adds a parameter ResponseMeteredLevel to the ResponseMetered annotation which decides whether 1xx/2xx/3xx/4xx/5xx meters, individual response code meters or both are created.

Port of #3043
joschi pushed a commit that referenced this pull request Feb 7, 2023
@dennyac
Add ResponseMeteredLevel parameter to ResponseMetered for jersey 3.0.x (
acb00e3 
#3115)

Adds a parameter ResponseMeteredLevel to the ResponseMetered annotation which decides whether 1xx/2xx/3xx/4xx/5xx meters, individual response code meters or both are created.

Port of #3043
joschi pushed a commit that referenced this pull request Feb 7, 2023
@dennyac
Add detailed response code meters for Jetty 9.x (#3133)
bdce01c 
This change set adds configurable individual response code meters in Jetty 9.x.

This change is backwards compatible and the default behavior will only include the existing 1xx/2xx/3xx/4xx/5xx meters.

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

@joschi joschi joschi approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
4.2.16
Development

Successfully merging this pull request may close these issues.
2 participants
@dennyac @joschi