feat(gax): accept clientOptions in createApiCall for telemetry tracing - #9176
feat(gax): accept clientOptions in createApiCall for telemetry tracing#9176shivanee-p wants to merge 1 commit into
Conversation
![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 introduces telemetry tracing options to ClientOptions and integrates them into createApiCall to determine if tracing is enabled. Key feedback includes explicitly checking if the GOOGLE_SDK_NODE_EXPERIMENTAL_O11Y_ENABLED environment variable equals 'true' to avoid false positives with other string values. Additionally, the console.log statements used for debugging tracing status should be removed to prevent polluting the standard output of consuming applications, and the corresponding unit tests that spy on these logs should be cleaned up.
f45d91d to
ae7b3a3
Compare
westarle
left a comment
westarle
left a comment
There was a problem hiding this comment.
I think we should consider CallSettings carefully. I would take the change to
cef268a to
39ae92a
Compare
a6454bc to
8baf444
Compare
8baf444 to
32dc2d6
Compare
danieljbruce
left a comment
danieljbruce
left a comment
There was a problem hiding this comment.
Do you have any more information about the CallSettings comment from Wes? Seems like it is okay to expose the options here, I just don't have enough context.
| import {warn} from './warnings'; | ||
|
|
||
| /** | ||
| * Checks if telemetry tracing is enabled |
There was a problem hiding this comment.
If the intent is to export this so that it is available to users then maybe a comment would be good about how to do that. Otherwise if it is used for the tests then maybe just moving it there.
| /** | ||
| * Whether to enable telemetry tracing for the client. | ||
| */ | ||
| enableTelemetryTracing?: boolean; |
There was a problem hiding this comment.
These two things were the agreed upon change to the API surface? I'm not too familiar with the project so just making sure.
| * The static trace context is information about the Google Cloud client library that is | ||
| * used to generate telemetry tracing information. | ||
| */ | ||
| internalTelemetryInfo?: StaticTraceContext; |
There was a problem hiding this comment.
I don't think we should add this to our public API if we can avoid it (e.g. by stuffing it directly into callsettings in the generated code)
westarle
left a comment
westarle
left a comment
There was a problem hiding this comment.
everything else lgtm if you can get a review from the team.
3 participants
Adds clientOptions support and telemetry gating logic to createApiCall in google-gax to support OpenTelemetry tracing.