feat: implement Observability Phase 1 (Config, GAX Network, Auth Spans) - #9573
feat: implement Observability Phase 1 (Config, GAX Network, Auth Spans)#9573cy-yun wants to merge 1 commit into
Conversation
2a16e32 to
14bda98
Compare
92de944 to
6c40ebc
Compare
| * A PSR-3 compliant logger. | ||
| * @type TracerProviderInterface|null $tracerProvider | ||
| * A tracer provider for OpenTelemetry. | ||
| * @type LoggerProviderInterface|null $loggerProvider |
There was a problem hiding this comment.
There does not seem to be any integration here with our existing PSR-3 compliant logger. I think this difference will be confusing to our customers (and also to me, I am confused as to what the difference is already!)
Libraries exist which serve as adapters between the two (see https://packagist.org/packages/open-telemetry/opentelemetry-logger-monolog). This may be a good way to support it in the short term. A more robust solution would be to do something similar to what we already have with HttpHandler, where we have a factory which builds our own adapter class, and so we can be confident that the methods we expect exist already.
I would like to see either 1) the practical difference between loggerProvider and logger explained in the description and variable name, OR 2) a way to bridge the two (preferred)
There was a problem hiding this comment.
thanks for the review!
I actually considered bridging this with PSR-3 (or using PSR-3 directly) in the initial design, but decided to keep loggerProvider (OpenTelemetry Logging API) separate from the existing PSR-3 logger.
The primary reason is trace-log correlation and structured attributes. The OpenTelemetry Logging API allows us to emit logs that are perfectly correlated with the current Trace ID and Span ID of the L4 network request (or T4 network spans), and it allows us to inject strongly-typed attributes (like gcp.errors.domain and gcp.errors.metadata.) directly into the log record's structured fields using LogRecordBuilder.
If we routed this through a PSR-3 adapter, we would lose this strict correlation and the native OTel structured logging capabilities required to link client-side traces to backend Google Cloud resources via App Hub.
I have updated the docblock in ClientOptions to explicitly clarify the practical difference between the two loggers so it's less confusing for users!
cy-yun
left a comment
There was a problem hiding this comment.
That's a very fair point about naming! Since PHP options are passed as untyped arrays, $options['logger'] and $options['loggerProvider'] do indeed look confusingly similar compared to languages with strictly-typed setters.
Looking at other languages:
- Node.js uses
tracerProvider: TracerProviderdirectly in its options object. - Python's
google-api-corehastracer_provideras a kwargs argument. - Java uses
setTracerProvider().
However, since most of these languages don't heavily mix PSR-3 equivalent unstructured loggers in the exact same configuration array, they don't hit this naming collision as hard.
To eliminate the frustration and make it crystal clear, I've renamed them to explicitly prefix the OpenTelemetry origin. They are now openTelemetryTracerProvider and openTelemetryLoggerProvider in ClientOptions.
This directly communicates their origin (OpenTelemetry) and cleanly separates them from the legacy PSR-3 logger. The changes have been pushed and rebased across all dependent PRs. What do you think?
I still think we could explore a way to resolve this issue, I cannot imagine the interfaces are that different
This is much better, I agree. Although if we are adding two new options to every single GAPIC client, I still would like to explore the purposes of these options in depth. Also, we will want to have a healthy comment block explaining their purpose.
I do not see any changes in this PR, can you make sure they were pushed? |
bshaffer
left a comment
There was a problem hiding this comment.
Awaiting update from requested changes to ClientOptions names and description
cy-yun
left a comment
There was a problem hiding this comment.
I scrutinized the PR against the design doc and OpenTelemetry best practices, and made the following fixes directly:
- Environment Variables Alignment: The design doc specifies
GOOGLE_CLOUD_<SIGNAL>_ENABLEDbut the code usedGOOGLE_SDK_PHP_<SIGNAL>_ENABLEDand an undocumented legacy fallbackGOOGLE_API_ENABLE_TELEMETRY. I removed the fictitious legacy fallback and corrected the env vars to match the design doc (GOOGLE_CLOUD_TRACING_ENABLED, etc.). - Auth Span Attributes:
AuthTracingMiddlewarewas settingrpc.system=httpandrpc.service=auth. Since OAuth token fetches are standard HTTP client requests and not RPCs, I corrected these to use the standard OpenTelemetry HTTP semantic conventions:http.request.methodandurl.full. - HTTP Status Code Typing: In
RestTransport.php, the HTTP status code was retrieved fromException::getCode(), which can sometimes be a string or0. Because OpenTelemetry attributes must be strictly typed, I explicitly cast the status code to an integer ((int) $statusCode) before emitting it to prevent validation errors in the Logger.
6c40ebc to
456925b
Compare
Implements Phase 1 Tasks 1-5 of the Client Libraries Observability v1 design: 1. Adds TelemetryConfiguration and openTelemetry options 2. Adds open-telemetry/api dependency to Core and Gax 3. Implements L4 network error logs in GAX 4. Implements tracing for Auth spans in RequestWrapper
456925b to
1c43507
Compare
Description
This PR combines and implements Phase 1 (Tasks 1-5) of the Client Libraries Observability v1 design into a single orthogonal chunk for easier review.
Changes
TelemetryConfigurationinGoogle\Cloud\Corefor securely parsing telemetry opt-in/opt-out configuration from environment variables. AddsopenTelemetryTracerProviderandopenTelemetryLoggerProviderconfigurations toClientOptionsinGoogle\ApiCore.open-telemetry/apidependency to bothCoreandGaxcomposer.jsonfiles.RestTransportandGrpcTransport) at theINFOseverity level. Attaches strongly-typed attributes likehttp.status_codeorrpc.grpc.status_code.AuthTracingMiddlewarewithinCore. Automatically instruments HTTP requests to emit anAuthRequestspan whenever a token is fetched or refreshed.Closes #9574
Closes #9582
Closes #9583