Migrate apollo_router_session_count_total metric to an OTel gauge#6495
Merged
goto-bus-stop merged 1 commit intodevfrom Jan 7, 2025
Merged
Migrate apollo_router_session_count_total metric to an OTel gauge#6495goto-bus-stop merged 1 commit intodevfrom
apollo_router_session_count_total metric to an OTel gauge#6495goto-bus-stop merged 1 commit intodevfrom
Conversation
Somehow missed this in #6476 <!-- [ROUTER-911] -->
Collaborator
✅ Docs Preview ReadyNo new or changed pages found. |
Contributor
|
@goto-bus-stop, please consider creating a changeset entry in |
|
CI performance tests
|
tninesling
approved these changes
Dec 20, 2024
Member
Author
|
Oh, there's a mistake in this PR. The guard for the total session count doesn't live long enough, so the value will be decremented too early. It's being fixed in #6527 |
goto-bus-stop
commented
Jan 13, 2025
| listener = &address | ||
| ); | ||
| } | ||
| let _guard = main_graphql_port.then(TotalSessionCountGuard::start); |
Member
Author
There was a problem hiding this comment.
Note this contains a mistake. The guard must be moved into the task::spawn call below, but it isn't, so the total session count metric is broken with this PR (it is immediately decremented before the next incoming request is handled.)
#6527 will fix this problem.
Member
Author
There was a problem hiding this comment.
That PR will be redone in a different way. #6539 fixes just this problem.
goto-bus-stop
added a commit
that referenced
this pull request
Jan 13, 2025
In #6495 I migrated this metric to an otel ObservableGauge. However the lifetime of the count guard was too short, so the value of the metric would almost always just be 0; and the lifetime of the instrument was too short, so even the wrong value of 0 would not actually be reported. Now the lifetime of the instrument is captured by all requests, so the instrument is kept even post-reload if a request is still being completed from the old schema. Also, a test verifies that the count increments and decrements as expected.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Somehow missed this in #6476
No changeset because it's in #6476 : )