-
Notifications
You must be signed in to change notification settings - Fork 821
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: cleanup setting config in instrumentations #2522
chore: cleanup setting config in instrumentations #2522
Conversation
There is no need to create a copy of the config object as this is done in base class. Remove defaulting to an empty config object at several places as this is done in the base class. Remove the intersection type with InstrumentationConfig as all config types extend this type. Avoid overriding the _config property in the grpc instrumentation.
Codecov Report
@@ Coverage Diff @@
## main #2522 +/- ##
==========================================
- Coverage 93.07% 93.07% -0.01%
==========================================
Files 140 140
Lines 5172 5169 -3
Branches 1111 1101 -10
==========================================
- Hits 4814 4811 -3
Misses 358 358
|
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 would add few other refactoring to have real typed getConfig
@@ -125,6 +116,10 @@ export class GrpcJsInstrumentation extends InstrumentationBase { | |||
]; | |||
} | |||
|
|||
override getConfig(): GrpcInstrumentationConfig { |
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.
are you overriding this just to have correct type, if so I would rather do it differently by refactoring abstract class - something like this
export abstract class InstrumentationAbstract<T = types.InstrumentationConfig>
implements types.Instrumentation {
protected _config: T;
constructor(
public readonly instrumentationName: string,
public readonly instrumentationVersion: string,
config: T
It would require small refactoring of init method as well as InstrumentationModuleDefinition
export interface InstrumentationModuleDefinition<T = any> {
But then you would not have to worry about overriding the getConfig to have typed config from instrumentation class WDYT ?
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.
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.
My stalled PR was stalled because it was more difficult to get typescript to be happy than I anticipated when I started it. For now, I think doing it this way is probably fine.
I would like to make the types smarter eventually. I know it should be possible if someone has the time and inclination.
Short description of the changes
There is no need to create a copy of the config object as this is done in base class.
Remove defaulting to an empty config object at several places as this is done in the base class.
Remove the intersection type with
InstrumentationConfig
as all config types extend this type.Avoid overriding the _config property in the grpc instrumentation.