Attach fid to captureError in telemetry
#9373
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
tonybaroneee
force-pushed
the
telemetry-capture-fid
branch
from
ed5be99 to
2ee5e0f
Compare
tonybaroneee
force-pushed
the
telemetry-capture-fid
branch
4 times, most recently
from
d4dcfba to
740a83f
Compare
| fid?: string; | ||
|
|
||
| constructor(public app: FirebaseApp, public installationsProvider: _FirebaseInstallationsInternal, public loggerProvider: LoggerProvider) { | ||
| void this._getFid(); |
There was a problem hiding this comment.
@hsubox76 Due to the nature of our API, the captureError method needs to stay synchronous, but retrieving the fid is async. This leads to the fid being set here on subsequent invocations of captureError, not the first. Any ideas on how to have the fid in place in time for the first captureError call while keeping that method sync? cc @andrewbrook
There was a problem hiding this comment.
Ah, I missed this. We could use the same model we have for dynamic headers, and only retrieve fid when doing the background request?
tonybaroneee
force-pushed
the
telemetry-capture-fid
branch
from
740a83f to
7ea2578
Compare
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (2,233,684 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
3 participants
Discussion
fid(or upserts if it doesn't yet exist) and attaches it to each error log entry that is captures by the telemetrycaptureErrorAPI.Testing
fidin log entry custom attributes payloadfidis provided in logs captured by a locally deployed test appAPI Changes