-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[data.search.session] Server telemetry on search sessions #91256
[data.search.session] Server telemetry on search sessions #91256
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a nit
}, | ||
}); | ||
usageCollection.registerCollector(collector); | ||
} catch (err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I dont see any reason to have this try/catch in place. usageCollection
is not optional in this function parameters and we never throw on collection registration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some error handling + unit tests
LGTM
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
) * [data.search.session] Server telemetry on search sessions * Update telemetry mappings * Added tests and logger Co-authored-by: Liza K <[email protected]>
…91410) * [data.search.session] Server telemetry on search sessions * Update telemetry mappings * Added tests and logger Co-authored-by: Liza K <[email protected]> Co-authored-by: Lukas Olson <[email protected]>
Summary
Resolves #62964.
Adds server-side telemetry for search sessions. Right now all it reports is the number of persisted/transient sessions, as well as the total count, but we can extend this to report more stats.