chore: consistent and typed use of instrumentation config #2289
Conversation
| getConfig: () => Required<GraphQLInstrumentationConfig>, | ||
| getConfig: () => GraphQLInstrumentationParsedConfig, |
There was a problem hiding this comment.
I'm not sure I follow the implications of this change.
Is this change why the config.depth! change below is necessary?
There was a problem hiding this comment.
The config.depth! non null assertion is by mistake. fixed 👍
The type Required<GraphQLInstrumentationConfig> is not accurate as the instrumentation parses some of the config options like depth, mergeItems etc, but some where still optional, like the enabled and ignoreTrivialResolveSpans which typescript didn't like.
The new implementation makes it so the type accurately reflects which fields are nullable and which are non-null after the DEFAULT_CONFIG merge.
Thanks, the TAV failing is from mongodb instrumentation, and is also failing on |
|
This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. |
|
This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. |
|
Updating your branch from main to get the mongodb TAV test fix. |
Oh wow it's adding and removing the label and always posting the message, that's quite annoying. Sorry about that. Edit: see #2316 |
|
This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. |
| @@ -80,3 +80,12 @@ export interface GraphQLInstrumentationConfig extends InstrumentationConfig { | |||
| */ | |||
| responseHook?: GraphQLInstrumentationExecutionResponseHook; | |||
| } | |||
|
|
|||
| // Utility type to make specific properties required | |||
| type RequireSpecificKeys<T, K extends keyof T> = T & { [P in K]-?: T[P] }; | |||
There was a problem hiding this comment.
can you please tell why this change was needed?, thx
After open-telemetry/opentelemetry-js#4659 is merged released and updated in contrib, we can benefit from the base class config functions being typed and required.
This PR implements and closes open-telemetry/opentelemetry-js#4668 by only accessing config from the getters and setters. After this one merges, I will make the
_configproperty private in base class so that it is not used accidentally.This PR apply the relevant changes to the contrib instrumentations, which makes everything consistent, short and well-typed (no type assertions).
There is still plenty of potential improvements and alignments for the way different instrumentations uses the config, but I want to leave this out of this PR so that it is not too large.
All the changes in this PR do not change behavior or logic, and should not be breaking in anyway