Skip to content
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

Merged
merged 13 commits into from
May 2, 2024

Conversation

blumamir
Copy link
Member

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:

  constructor(config: FooInstrumentationConfig = {}) {
    super('@opentelemetry/instrumentation-foo', VERSION, config);
  }

@blumamir
Copy link
Member Author

After open-telemetry/opentelemetry-js#4659 lands, we can introduce even more consistency and cleanups on the handling of config in instrumentations.
This PR only targets the constructor and super invocation usage

@@ -65,9 +64,7 @@ const DEFAULT_CONFIG: GraphQLInstrumentationConfig = {
const supportedVersions = ['>=14 <17'];

export class GraphQLInstrumentation extends InstrumentationBase {
constructor(
config: GraphQLInstrumentationConfig & InstrumentationConfig = {}
Copy link
Member Author

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

Copy link

codecov bot commented Apr 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.47%. Comparing base (dfb2dff) to head (fb36d11).
Report is 98 commits behind head on main.

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     
Files Coverage Δ
...lugins/node/instrumentation-amqplib/src/amqplib.ts 90.40% <100.00%> (ø)
.../instrumentation-dataloader/src/instrumentation.ts 99.06% <100.00%> (-0.02%) ⬇️
...ode/instrumentation-tedious/src/instrumentation.ts 92.39% <100.00%> (-0.09%) ⬇️
plugins/node/instrumentation-undici/src/undici.ts 96.23% <100.00%> (ø)
...-instrumentation-aws-lambda/src/instrumentation.ts 93.60% <100.00%> (-0.08%) ⬇️
...entelemetry-instrumentation-aws-sdk/src/aws-sdk.ts 95.02% <100.00%> (-0.11%) ⬇️
...etry-instrumentation-bunyan/src/instrumentation.ts 97.89% <100.00%> (-0.81%) ⬇️
...try-instrumentation-connect/src/instrumentation.ts 98.98% <100.00%> (-0.03%) ⬇️
...lemetry-instrumentation-dns/src/instrumentation.ts 94.66% <100.00%> (+11.12%) ⬆️
...try-instrumentation-express/src/instrumentation.ts 100.00% <100.00%> (ø)
... and 19 more

... and 15 files with indirect coverage changes

@@ -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;
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Member

@JamieDanielson JamieDanielson left a 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.

@blumamir
Copy link
Member Author

Looks like we are just missing Mongoose and Koa, feel free to grab this commit if it's easiest. Otherwise this looks good.

right! I missed those. thanks for spotting. added them to the PR

@trentm
Copy link
Contributor

trentm commented Apr 30, 2024

The TAV test for Node.js v16 failed on instr-undici with undici@5.28.2.

https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/8899428327/job/24438782853?pr=2162

-- required packages ["undici@5.28.2"]
-- installing ["undici@5.28.2"]
-- running test "npm run test" with undici (env: {})

> @opentelemetry/instrumentation-undici@0.2.0 test
> nyc ts-mocha -p tsconfig.json test/**/*.test.ts



  UndiciInstrumentation `fetch` tests
    disable()
      - should not create spans when disabled
    enable()
      - should create valid spans even if the configuration hooks fail
      - should create valid spans with empty configuration
      - should create valid spans with the given configuration
      - should not create spans without parent if required in configuration
      - should not create spans with parent if required in configuration
      - should capture errors using fetch API
      - should capture error if fetch request is aborted

  UndiciInstrumentation metrics tests
    with fetch API
      - should report "http.client.request.duration" metric
      - should have error.type in "http.client.request.duration" metric

  UndiciInstrumentation `undici` tests
    disable()
      ✓ should not create spans when disabled (38ms)
    enable()
      ✓ should ignore requests based on the result of ignoreRequestHook
      - should create valid spans for different request methods
      ✓ should create valid spans for "request" method
      ✓ should create valid spans for "fetch" method
      ✓ should create valid spans for "stream" method
      ✓ should create valid spans for "dispatch" method
      ✓ should create valid spans even if the configuration hooks fail
      ✓ should not create spans without parent if required in configuration
      ✓ should create spans with parent if required in configuration
      1) should capture errors while doing request
      ✓ should capture error if undici request is aborted


  10 passing (2s)
  11 pending
  1 failing

  1) UndiciInstrumentation `undici` tests
       enable()
         should capture errors while doing request:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/instrumentation-undici/test/undici.test.ts)
      at listOnTimeout (node:internal/timers:559:17)
      at processTimers (node:internal/timers:502:7)

I'll re-run that test. I can't see how this PR would ahve caused it.

@blumamir blumamir merged commit 2ade5ae into open-telemetry:main May 2, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment