fix: Add Firebase dashboard resource attributes for Python telemetry visibility#4654
fix: Add Firebase dashboard resource attributes for Python telemetry visibility#4654zarinn3pal wants to merge 2 commits intofirebase:mainfrom
Conversation
Summary of ChangesHello @zarinn3pal, 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 enhances the Python telemetry setup by integrating Firebase-compatible resource attributes. This ensures that telemetry data from Python applications is correctly recognized and displayed within the Firebase Genkit monitoring dashboard, aligning its behavior with existing JavaScript and Go implementations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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.
Code Review
This pull request aims to add Firebase-compatible resource attributes to telemetry data for better visibility in dashboards. The changes include refactoring resource creation into a shared method, which is a good improvement for code reuse and consistency. The resource attributes are correctly applied to metrics. However, there is a critical issue where the new resource attributes are not applied to traces due to an incomplete implementation in the trace exporter. This means the primary goal of the PR is not fully achieved for tracing. Additionally, the related test case should be strengthened to verify that the resource attributes are correctly attached to exported spans.
| error_handler: Optional callback invoked when export errors occur. | ||
| resource: Optional OpenTelemetry resource for consistent attribution. | ||
| """ | ||
| self.resource = resource |
There was a problem hiding this comment.
The resource parameter is stored in self.resource but it's never used. This means the Firebase-compatible resource attributes will not be applied to traces, and they won't appear correctly in the Firebase dashboard. The resource needs to be attached to the spans before they are exported.
The standard OpenTelemetry approach is to configure the TracerProvider with a Resource. However, Genkit's tracing initialization seems to make this difficult to do from a plugin.
A possible fix within this exporter would be to wrap the span and override its resource property before exporting. This would involve creating a custom ReadableSpan wrapper that merges self.resource with the span's existing resource.
This is a significant issue that prevents the PR from working as intended for traces.
| # Verify resource detection was called for traces (Firebase dashboard needs this) | ||
| mock_detector.assert_called_once_with(raise_on_error=True) | ||
| mock_detector.return_value.detect.assert_called_once() |
There was a problem hiding this comment.
This test correctly verifies that resource detection is performed. However, it doesn't verify that the detected and created resource is actually applied to the exported traces. As noted in another comment, the resource object is currently not being used by the trace exporter, which is a critical bug.
The test should be extended to assert that the exported spans contain the merged resource attributes. This could be done by inspecting the spans passed to the mocked GenkitGCPExporter.export method.
ef8b31e to
b67d85b
Compare
|
Hmm this doesn't seem right. I was able to get some stuff to show in AIM dashboard just by fixing the logging pattern. |
This PR adds dashboard resource attributes so that AIM dashboard gets populated