Repository navigation
feat(telemetry): tee spans to OTEL_EXPORTER_OTLP_ENDPOINT - #7552
beyondszine wants to merge 1 commit into
Conversation
GCP telemetry setup exports spans only to telemetry.googleapis.com and ignores the standard OTEL_EXPORTER_OTLP_ENDPOINT. Append an additional span processor to the OTelHooks list when the standard OTLP endpoint variable is set, so an existing OTLP pipeline receives spans alongside the GCP exporters. Opt-in, additive, standard OpenTelemetry configuration.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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 attempts to add support for user-configured OTLP endpoints within the GCP telemetry exporter setup. However, the review feedback correctly identifies that this change is redundant and introduces a bug where spans are exported twice, as the telemetry setup already automatically registers generic OTel exporters when these environment variables are present. It is recommended to revert these changes and keep the GCP exporter strictly scoped to GCP-specific logic.
| if user_span_processor := _maybe_get_configured_otlp_span_processor(): | ||
| span_processors.append(user_span_processor) |
There was a problem hiding this comment.
This change is redundant and introduces a bug where spans are exported twice to the configured OTLP endpoint.\n\nIn src/google/adk/telemetry/setup.py, the maybe_set_otel_providers function already automatically appends generic OTel exporters (including the OTLP span exporter) by calling _get_otel_exporters():\n\npython\n # Add generic OTel exporters based on OTel env variables.\n hooks_to_setup.append(_get_otel_exporters())\n\n\nIf a user configures OTEL_EXPORTER_OTLP_ENDPOINT and calls maybe_set_otel_providers([get_gcp_exporters(...)]), both get_gcp_exporters and _get_otel_exporters will register a BatchSpanProcessor(OTLPSpanExporter()). This results in duplicate span processors being registered to the TracerProvider, causing every span to be exported twice to the user's OTLP pipeline.\n\nSince the existing setup already correctly tees spans to the standard OTLP endpoint when the environment variables are set, we should revert this change and keep get_gcp_exporters strictly scoped to GCP-specific exporters.
| def _maybe_get_configured_otlp_span_processor() -> Optional[SpanProcessor]: | ||
| """Returns a span processor for a user-configured OTLP endpoint, or None when | ||
| OTEL_EXPORTER_OTLP_(TRACES_)ENDPOINT is not set. | ||
|
|
||
| Lets an existing OTLP pipeline receive spans alongside the GCP exporters. The | ||
| OTLPSpanExporter reads OTEL_EXPORTER_OTLP_* (endpoint, headers, timeout) | ||
| itself; this only wires it into the provider. | ||
| """ | ||
| if not ( | ||
| os.environ.get("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT") | ||
| or os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT") | ||
| ): | ||
| return None | ||
| try: | ||
| from opentelemetry.exporter.otlp.proto.http.trace_exporter import OTLPSpanExporter | ||
| except (ImportError, AttributeError): | ||
| logger.warning( | ||
| "OTEL_EXPORTER_OTLP_ENDPOINT is set but" | ||
| " opentelemetry-exporter-otlp-proto-http is not installed;" | ||
| " skipping the additional OTLP span exporter." | ||
| ) | ||
| return None | ||
| return BatchSpanProcessor(OTLPSpanExporter()) |
There was a problem hiding this comment.
This helper function is no longer needed since the OTLP span exporter is already configured and managed in src/google/adk/telemetry/setup.py via _get_otel_exporters().
|
Thanks @gemini-code-assist -- you're right. The actual gap is on the Vertex SDK side: |
|
Thanks for the update, @beyondszine. That makes perfect sense. Avoiding double-exporting is crucial for telemetry accuracy, and identifying that the Vertex SDK setup path was the actual culprit is a great catch. Good luck with the fix in the other repository! |
get_gcp_exportersbuilds span processors only for the GCP telemetry endpoint and ignores the standardOTEL_EXPORTER_OTLP_ENDPOINT. Users who want their agent's traces in an existing OTLP pipeline (a collector, an APM, a second backend) have no additive option.This appends an additional span processor to the
OTelHooksspan-processor list when the standard OTLP endpoint variable is set, so an existing OTLP pipeline receives spans alongside the GCP exporters. It is opt-in, uses standard OpenTelemetry configuration, and is purely additive.OTLPSpanExporter()readsOTEL_EXPORTER_OTLP_*(endpoint, headers, timeout) itself.Note:
get_gcp_exportersis GCP-scoped, so maintainers may prefer this live intelemetry/setup.py(maybe_set_otel_providers) instead -- happy to move it.