feat(o11y): introduce observability flags needed within the generator - #9052
feat(o11y): introduce observability flags needed within the generator#9052shivanee-p wants to merge 3 commits 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 support for an enableTelemetryTracing option across the generator, schema, and client interface packages, including adding the option to ClientOptions and introducing an InternalTelemetry interface. Feedback suggests updating the parameter parsing in the generator to support both hyphenated and snake_case parameter names for consistency with other flags, and to only set the property to true when explicitly configured.
| private readEnableTelemetryTracing() { | ||
| this.enableTelemetryTracing = this.paramMap['enable_telemetry_tracing'] === 'true'; | ||
| } |
There was a problem hiding this comment.
To maintain consistency with other parameter parsing methods in this class (such as readLegacyProtoLoad), we should only set the property to true if the parameter is explicitly provided as 'true'. Additionally, we should support the hyphenated parameter name enable-telemetry-tracing to match the repository's naming convention for generator flags.
private readEnableTelemetryTracing() {
if (this.paramMap['enable-telemetry-tracing'] === 'true' || this.paramMap['enable_telemetry_tracing'] === 'true') {
this.enableTelemetryTracing = true;
}
}There was a problem hiding this comment.
I think the way that flags are read in and manipulated from the .yaml file aren't consistent depending on what level of flag it is. Because this will end up being the name of an attribute within the extra_protoc_parameters field, it wouldn't get changed to kebab case, so it will remain with the underscores.
There was a problem hiding this comment.
It looks like they are normalized to kebab-case:
Could you please add a test for this (getParamMap & readX's) to make sure we are getting the value as expected?
| private readEnableTelemetryTracing() { | ||
| this.enableTelemetryTracing = this.paramMap['enable_telemetry_tracing'] === 'true'; | ||
| } |
There was a problem hiding this comment.
It looks like they are normalized to kebab-case:
Could you please add a test for this (getParamMap & readX's) to make sure we are getting the value as expected?
There was a problem hiding this comment.
please move the gax changes to a new PR if possible.
1926b69 to
3330e73
Compare
c7259a4 to
4159976
Compare
2 participants
Adds support for the o11y feature flag to
gapic-generator-typescriptand updates client options interface definitions ingoogle-gax