-
Notifications
You must be signed in to change notification settings - Fork 664
fix: Add Firebase dashboard resource attributes for Python telemetry visibility #4654
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -187,7 +187,7 @@ def test_add_gcp_telemetry_disable_traces() -> None: | |
|
|
||
|
|
||
| def test_add_gcp_telemetry_disable_metrics() -> None: | ||
| """Test that disable_metrics=True skips metrics export (JS/Go parity).""" | ||
| """Test that disable_metrics=True skips metrics export but still enables traces with resource detection.""" | ||
| with ( | ||
| mock.patch.dict(os.environ, {EnvVar.GENKIT_ENV: GenkitEnvironment.PROD}), | ||
| patch('genkit.plugins.google_cloud.telemetry.config.GenkitGCPExporter'), | ||
|
|
@@ -199,17 +199,24 @@ def test_add_gcp_telemetry_disable_metrics() -> None: | |
| patch('genkit.plugins.google_cloud.telemetry.config.PeriodicExportingMetricReader') as mock_reader, | ||
| patch('genkit.plugins.google_cloud.telemetry.config.metrics'), | ||
| ): | ||
| # Configure mock detector to return a mock resource | ||
| mock_resource = mock.MagicMock() | ||
| mock_detector.return_value.detect.return_value = mock_resource | ||
|
|
||
| from genkit.plugins.google_cloud.telemetry.tracing import add_gcp_telemetry | ||
|
|
||
| # Call with disable_metrics=True (JS/Go: disableMetrics) | ||
| add_gcp_telemetry(disable_metrics=True) | ||
|
|
||
| # Verify metrics exporter was NOT created | ||
| mock_detector.assert_not_called() | ||
| # Verify metrics exporters were NOT created (metrics disabled) | ||
| mock_metric_exp.assert_not_called() | ||
| mock_genkit_metric.assert_not_called() | ||
| mock_reader.assert_not_called() | ||
|
|
||
| # 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() | ||
|
Comment on lines
+216
to
+218
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 |
||
|
|
||
|
|
||
| def test_add_gcp_telemetry_custom_metric_interval() -> None: | ||
| """Test that metric_export_interval_ms is passed correctly (JS/Go parity).""" | ||
|
|
||
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.
The
resourceparameter is stored inself.resourcebut 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
TracerProviderwith aResource. 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
resourceproperty before exporting. This would involve creating a customReadableSpanwrapper that mergesself.resourcewith the span's existing resource.This is a significant issue that prevents the PR from working as intended for traces.