-
Notifications
You must be signed in to change notification settings - Fork 987
Add support for async attributes in temeletry otel logger #9373
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
Conversation
|
Summary of ChangesHello @tonybaroneee, 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 a significant enhancement to the telemetry system by integrating Firebase Installation IDs (FIDs) into error logging. By attaching a unique FID to each captured error, the system gains the ability to identify and correlate errors more effectively, providing a clearer picture of user-specific issues and improving error analysis capabilities. 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 the Firebase Installation ID (fid) to error logs, which will serve as a user identifier. The changes are well-structured and the tests are updated accordingly. My main feedback is on the implementation of fid fetching in TelemetryService, which uses a fire-and-forget approach. This can lead to a race condition where early errors are logged without an fid, and the error handling for the fetch operation could be improved for better debuggability. I've left a specific comment with a suggestion.
ed5be99 to
2ee5e0f
Compare
d4dcfba to
740a83f
Compare
740a83f to
7ea2578
Compare
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
a9e1ad8 to
6cbb429
Compare
1b2654c to
e66c193
Compare
captureError in telemetryd7e58db to
af87ba0
Compare
6d64a6b to
8aa5d9f
Compare
Discussion
DynamicLogAttributeProvider(similar toDynamicHeaderProviderwhich allows us to resolve async attributes (i.e. installation id) during the background otel export. This lets us keep ourcaptureErrorAPI synchronous. It's extensible enough to easily support more async attributes in the future.InstallationIdProvider, which attaches the client's firebase installation id to each error log entry.Testing
InstallationIdProvideruser.idattribute is set correctly in logs captured by a locally deployed test app.API Changes