-
Notifications
You must be signed in to change notification settings - Fork 49
add telemetry hook implementation guidelines #348
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael Beemer <[email protected]>
Signed-off-by: Michael Beemer <[email protected]>
Summary of ChangesHello @beeme1mr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive guidelines for implementing observability hooks that emit OpenTelemetry signals during feature flag evaluations. The new guide, added to the specification's appendix, aims to standardize how maintainers write telemetry hooks across various SDKs by detailing signal emission patterns, hook lifecycle responsibilities, attribute transformations, and privacy considerations for flag values. This will facilitate consistent and robust telemetry integration. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds a comprehensive and well-structured guide for implementing telemetry hooks. The documentation covers important aspects like signal emission patterns, hook lifecycle, attribute transformations, privacy, configuration, and error handling. I've added a couple of suggestions to improve the clarity and precision of the guidelines, which is important for a specification document. Overall, this is a valuable addition.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Michael Beemer <[email protected]>
Signed-off-by: Michael Beemer <[email protected]>
Signed-off-by: Michael Beemer <[email protected]>
|
@beeme1mr Please clarify what the span event name should be - |
Signed-off-by: Michael Beemer <[email protected]>
Signed-off-by: Michael Beemer <[email protected]>
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! Just two minor questions in case they're typos.
|
|
||
| For consistency across implementations, consider supporting a common set of configuration options: | ||
|
|
||
| - `excludeExceptions` (boolean): Whether to exclude exception recording in telemetry |
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.
Does this include both error message and error type? Can we clarify that here?
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.
Okay, I'll try and clarify what I mean. Here's an example for JS.
https://github.com/open-feature/js-sdk-contrib/pull/1372/files#diff-8d1358ebed8b9039db57f7495ae547872e4f246efe0f198838e98a2d2208cd22R39-R41
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.
I couldn't think of a way to rephrase this to make it more clear. Do you have any recommendations? The goal of the setting it control whether or not the hook should capture exception via the OTel API.
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.
Ok I see. Would it make sense to include a link to this?
https://opentelemetry.io/docs/specs/semconv/exceptions/exceptions-logs/
Co-authored-by: André Silva <[email protected]> Signed-off-by: Michael Beemer <[email protected]>
Co-authored-by: André Silva <[email protected]> Signed-off-by: Michael Beemer <[email protected]>
Signed-off-by: Michael Beemer <[email protected]>
This PR
Notes
This is based on the updated telemetry hook @lukas-reining wrote. I tried to capture as much context as possible so that other maintainers can understand how the hook should be written.
Since this is in the appendix section, I decided to make the guide human-readable instead of just a list of requirements.
Follow-up Tasks
Create implementation issues for all SDKs based on these guidelines.