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

Remove metrics endpoints #6283

Merged
merged 17 commits into from
Jun 20, 2024
Merged

Conversation

eddyashton
Copy link
Member

Instead of us maintaining a metrics object, we provide method event hooks so that registries that want to do this can do it themselves, or else just log. In the governance registry, we log every request and its execution time. We also have some custom tracking of jwt_refresh, because it's externally triggered and otherwise hard to track.

For apps I currently log everything, but I think that's too verbose. Opening as draft to see what the perf impact is. Think the end result will be that apps can log everything, if they want.

@eddyashton
Copy link
Member Author

eddyashton commented Jun 20, 2024

Rough measure of the perf impact of logging every (/app) request as it completes:

$ for i in {1..5}; do ctest -VV -R pi_basic_virtual | grep "Average throughput"; done
81: Average throughput: 57120.40 tx/s
81: Average throughput: 57197.73 tx/s
81: Average throughput: 56755.04 tx/s
81: Average throughput: 56493.09 tx/s
81: Average throughput: 56622.10 tx/s

$ wc -l workspace/pi_basic_virtual_0/out 
100197 workspace/pi_basic_virtual_0/out

Compared to an equivalent build where we don't log:

$ for i in {1..5}; do ctest -VV -R pi_basic_virtual | grep "Average throughput"; done
81: Average throughput: 62644.75 tx/s
81: Average throughput: 60552.30 tx/s
81: Average throughput: 61906.70 tx/s
81: Average throughput: 61003.92 tx/s
81: Average throughput: 62806.77 tx/s

$ wc -l workspace/pi_basic_virtual_0/out
181 workspace/pi_basic_virtual_0/out

I think this is a significant enough hit that we don't want it by default, and the current implementation (where apps can easily opt in to logging or gathering metrics as they wish) seems fine. So I'm no longer logging in UserEndpointRegistry.

Addendum:

The previous "without" numbers were from main, where we still have the metrics code including mutex locks on every request completion. Testing on this branch where we've removed that mutex suggests we might get a small but noticeable speedup from this:

$ for i in {1..5}; do ctest -VV -R pi_basic_virtual | grep "Average throughput"; done
81: Average throughput: 62528.69 tx/s
81: Average throughput: 63181.41 tx/s
81: Average throughput: 62403.94 tx/s
81: Average throughput: 62519.58 tx/s
81: Average throughput: 62973.88 tx/s

@eddyashton eddyashton marked this pull request as ready for review June 20, 2024 12:01
@eddyashton eddyashton requested a review from a team June 20, 2024 12:01
@eddyashton eddyashton changed the title [DRAFT] Remove metrics endpoints Remove metrics endpoints Jun 20, 2024
@eddyashton eddyashton merged commit 647fbbd into microsoft:main Jun 20, 2024
21 checks passed
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.

2 participants