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 5 commits 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
There was a problem hiding this comment.
I made the following fixes
- 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.
77f7ce0 to
48af3f8
Compare
c42b831 to
7c957e0
Compare
7c957e0 to
4090eae
Compare
3d74452 to
fd43fa5
Compare
4e431f3 to
4fb640b
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.BREAKING_CHANGE_REASON=Adding optional telemetry parameters to internal constructors