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-5017 Add authorization metrics #5074

Merged
merged 11 commits into from Mar 28, 2024

Conversation

yashmurty
Copy link
Contributor

@yashmurty yashmurty commented Jul 26, 2023

Motivation:

Fixing an open improvement issue. #5017

Modifications:

  • Modified the AuthService.java file to use MoreMeters.newTimer.
  • Updated the tests. I think some are still remaining to be fixed, so opening the PR as draft.
    • Any feedback is welcome as it's my first time! 😄

Result:

The metrics would look like:

# HELP armeria_server_auth_seconds  
# TYPE armeria_server_auth_seconds summary
armeria_server_auth_seconds_count 1.0
armeria_server_auth_seconds_sum 0.018437417
# HELP armeria_server_auth_seconds_max  
# TYPE armeria_server_auth_seconds_max gauge
armeria_server_auth_seconds_max 0.018437417

@trustin
Copy link
Member

trustin commented Jul 28, 2023

I don't see a test case that actually checks whether the metric is recorded. Could you add one? Thanks! 🙇

@yashmurty yashmurty force-pushed the Fix-5017-add-auth-metrics branch 2 times, most recently from 6a09452 to 586bc0e Compare August 8, 2023 08:48
@yashmurty yashmurty requested a review from trustin August 8, 2023 08:49
@yashmurty yashmurty force-pushed the Fix-5017-add-auth-metrics branch 2 times, most recently from 836785f to ba55e14 Compare August 8, 2023 09:09
@yashmurty yashmurty marked this pull request as ready for review August 10, 2023 06:26
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Looks good overall, left a minor comment

@yashmurty yashmurty requested a review from jrhee17 August 22, 2023 07:08
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Sorry about the delay 😅 I think once test related comments are addressed this PR should be good for merging 🙇

.addService(new AuthenticatedHttpJsonTranscodingTestService())
.enableHttpJsonTranscoding(true)
.build();
.addService(new AuthenticatedHttpJsonTranscodingTestService())
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind reverting incorrect indentations in this file?

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 74.04%. Comparing base (bc2454a) to head (f084185).
Report is 2 commits behind head on main.

Files Patch % Lines
.../com/linecorp/armeria/server/auth/AuthService.java 86.66% 0 Missing and 2 partials ⚠️
...necorp/armeria/server/auth/AuthServiceBuilder.java 33.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5074       +/-   ##
===========================================
+ Coverage        0   74.04%   +74.04%     
- Complexity      0    20797    +20797     
===========================================
  Files           0     1801     +1801     
  Lines           0    76594    +76594     
  Branches        0     9762     +9762     
===========================================
+ Hits            0    56715    +56715     
- Misses          0    15257    +15257     
- Partials        0     4622     +4622     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@minwoox minwoox added this to the 1.27.0 milestone Oct 11, 2023
@ikhoon ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 5, 2024
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Sorry it took a while 😄 Looks good to me 👍 🙇 👍

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks, @yashmurty!

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks, @yashmurty! 🚀👍

@jrhee17 jrhee17 merged commit eb9470c into line:main Mar 28, 2024
18 checks passed
@yashmurty yashmurty deleted the Fix-5017-add-auth-metrics branch March 28, 2024 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a metric that measures the time for authorizing
5 participants