-
Notifications
You must be signed in to change notification settings - Fork 534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: align config constructor pattern across instrumentations #2162
Conversation
After open-telemetry/opentelemetry-js#4659 lands, we can introduce even more consistency and cleanups on the handling of config in instrumentations. |
@@ -65,9 +64,7 @@ const DEFAULT_CONFIG: GraphQLInstrumentationConfig = { | |||
const supportedVersions = ['>=14 <17']; | |||
|
|||
export class GraphQLInstrumentation extends InstrumentationBase { | |||
constructor( | |||
config: GraphQLInstrumentationConfig & InstrumentationConfig = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it because GraphQLInstrumentationConfig
already extends InstrumentationConfig
so it was redundant
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2162 +/- ##
==========================================
- Coverage 90.97% 90.47% -0.50%
==========================================
Files 146 147 +1
Lines 7492 7594 +102
Branches 1502 1589 +87
==========================================
+ Hits 6816 6871 +55
- Misses 676 723 +47
|
@@ -77,8 +77,10 @@ export class AwsLambdaInstrumentation extends InstrumentationBase { | |||
private _traceForceFlusher?: () => Promise<void>; | |||
private _metricForceFlusher?: () => Promise<void>; | |||
|
|||
constructor(protected override _config: AwsLambdaInstrumentationConfig = {}) { | |||
super('@opentelemetry/instrumentation-aws-lambda', VERSION, _config); | |||
protected override _config!: AwsLambdaInstrumentationConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another bit we may want to align, which is the way we type config in our instrumentations. Here we use the override
keyword so we tell typescript to treat the property as an AwsLambdaInstrumentationConfig
type. And in other instrumentations we have a getter that casts the config property to the type we need
getConfig(): AwsLambdaInstrumentationConfig {
return this._config as AwsLambdaInstrumentationConfig;
}
Should we encourage one over he other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once open-telemetry/opentelemetry-js#4659 lands, we can cleanup all the config handling variations as the base class would already be typed correctly. Eager to apply this cleanup in all contrib instrumentations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we are just missing Mongoose and Koa, feel free to grab this commit if it's easiest. Otherwise this looks good.
plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
right! I missed those. thanks for spotting. added them to the PR |
The TAV test for Node.js v16 failed on instr-undici with [email protected].
I'll re-run that test. I can't see how this PR would ahve caused it. |
There are dozens of instrumentations in this repo, written at different times by different people.
Few different patterns emerged on how to handle the config object in Instrumentation constructor. This PR aims to introduce a common pattern to have consistency across the repo.
This PR aligns all the instrumentations to use: