chore: experimental 0.51.0, remove instrumentations generic type to align with upstream #2091

blumamir · 2024-04-11T11:13:28Z

This PR applied the changes in open-telemetry/opentelemetry-js#4598 to the contrib repo.

Once we merge the upstream change in code and release a core version, this PR can be used to apply the removal of the generic type from InstrumentationBase, InstrumentationNodeModuleDefinition and InstrumentationNodeModuleFile.

It was tested to npm run compile against the up-to-date @opentelemetry/instrumentation package with npm link locally.

BEGIN_COMMIT_OVERRIDE
feat(deps): update otel-js to 0.51.0
feat: remove generic type from instrumentations
END_COMMIT_OVERRIDE

codecov · 2024-04-24T15:59:32Z

Codecov Report

Attention: Patch coverage is 96.05263% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 90.34%. Comparing base (dfb2dff) to head (8a2a0d3).
Report is 70 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2091      +/-   ##
==========================================
- Coverage   90.97%   90.34%   -0.63%     
==========================================
  Files         146      147       +1     
  Lines        7492     7678     +186     
  Branches     1502     1575      +73     
==========================================
+ Hits         6816     6937     +121     
- Misses        676      741      +65

Files	Coverage Δ
...de/instrumentation-cucumber/src/instrumentation.ts	`92.30% <100.00%> (ø)`
.../instrumentation-dataloader/src/instrumentation.ts	`99.08% <ø> (ø)`
...ode/instrumentation-tedious/src/instrumentation.ts	`92.47% <100.00%> (ø)`
...entelemetry-instrumentation-aws-sdk/src/aws-sdk.ts	`95.12% <100.00%> (ø)`
...etry-instrumentation-bunyan/src/instrumentation.ts	`97.91% <100.00%> (-0.79%)`	⬇️
...y-instrumentation-cassandra/src/instrumentation.ts	`81.39% <100.00%> (ø)`
...try-instrumentation-connect/src/instrumentation.ts	`99.01% <100.00%> (ø)`
...lemetry-instrumentation-dns/src/instrumentation.ts	`94.93% <100.00%> (+11.39%)`	⬆️
...try-instrumentation-express/src/instrumentation.ts	`100.00% <100.00%> (ø)`
...try-instrumentation-fastify/src/instrumentation.ts	`95.48% <ø> (ø)`
... and 21 more

... and 9 files with indirect coverage changes

blumamir · 2024-04-24T16:53:59Z

plugins/node/instrumentation-dataloader/src/instrumentation.ts

-        // cast it to module definition of unknown type to avoid exposing Dataloader types on public APIs
-      ) as InstrumentationNodeModuleDefinition<unknown>,


Since this comment is here, I think it's not safe to type the moduleExports with the now-removed generic type, which does not give a lot of value IMO.

blumamir · 2024-04-24T17:03:42Z

plugins/node/opentelemetry-instrumentation-bunyan/src/instrumentation.ts

@@ -45,7 +43,7 @@ export class BunyanInstrumentation extends InstrumentationBase<

  protected init() {
    return [
-      new InstrumentationNodeModuleDefinition<typeof BunyanLogger>(


The module parameter for the patch is already typed as any, might be improved in a followup PR.

blumamir · 2024-04-24T17:05:19Z

plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts

@@ -52,7 +52,7 @@ export class ConnectInstrumentation extends InstrumentationBase<Server> {

  init() {
    return [
-      new InstrumentationNodeModuleDefinition<any>(


This is already typed as any so the moduleExports type is not typed at the moment. Can be improved in a future PR. want to keep this one minimal to the relevant changes

blumamir · 2024-04-24T17:08:05Z

plugins/node/opentelemetry-instrumentation-dns/src/instrumentation.ts

-  Dns,
-  DnsPromises,


these imports invalidated the internal-types.ts guarntee, as they were exported in the public API from an internal file

blumamir · 2024-04-24T17:32:59Z

plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts

@@ -95,10 +95,8 @@ export class GraphQLInstrumentation extends InstrumentationBase {
    return module;
  }

-  private _addPatchingExecute(): InstrumentationNodeModuleFile<
-    typeof graphqlTypes


here the patch function argument is already typed as any so the generic type had no effect. trying to type moduleExports yields a ts error

blumamir · 2024-04-24T17:37:02Z

plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts

@@ -71,7 +69,7 @@ export class KnexInstrumentation extends InstrumentationBase<any> {
  }

  private getRunnerNodeModuleFileInstrumentation(basePath: string) {
-    return new InstrumentationNodeModuleFile<typeof knex>(


The patch function is already typed with any, so this has no effect

blumamir · 2024-04-24T17:37:47Z

plugins/node/opentelemetry-instrumentation-koa/src/instrumentation.ts

@@ -52,7 +52,7 @@ export class KoaInstrumentation extends InstrumentationBase<typeof koa> {
  }

  protected init() {
-    return new InstrumentationNodeModuleDefinition<typeof koa>(


the generic has no effect, moudleExports is typed with any

blumamir · 2024-04-24T17:40:14Z

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts

        'mongodb',
        ['>=3.3 <4'],
        undefined,
        undefined,
        [
-          new InstrumentationNodeModuleFile<WireProtocolInternal>(


The patch function, v3PatchConnection is already typed correctly

blumamir · 2024-04-24T17:40:44Z

plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts

        'mongodb',
        ['4.*', '5.*', '6.*'],
        undefined,
        undefined,
        [
-          new InstrumentationNodeModuleFile<V4Connection>(


The patch function, v4PatchConnectionCallback is already typed as any

blumamir · 2024-04-24T17:44:26Z

plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts

@@ -52,7 +52,7 @@ export class PgInstrumentation extends InstrumentationBase {
  }

  protected init() {
-    const modulePG = new InstrumentationNodeModuleDefinition<typeof pgTypes>(


moudle below is already any this type has no effect

blumamir · 2024-04-24T17:55:14Z

Thanks @JamieDanielson

did another round of fixes thanks to your great comment, added back the types to cucumber, tedious, dns, graphql, hapi, Memcached, mysql, net, winston. If you spot anyone I missed please comment.

I also added comments in the PR documenting few places where the generic type had no effect because the moduleExports argument type was override with any

My intention in this PR is to keep everything in the same state it is now with types, and only introduce the changes from core. I support a followup work for deciding on guidelines on how the types should be used, and then align all instrumentations according to some common rules. I guess we still need some freedom to define as any if the type is too complex or impossible to acquire, and also allow typing the argument where it makes sense and is accessible with minimum issues.

…ion-generic

blumamir · 2024-04-24T18:22:08Z

The browsers test currently fails, seems unrelated to this PR.
@pichlermarc mentioned it might be related to open-telemetry/opentelemetry-js#4486

pichlermarc · 2024-04-24T20:42:22Z

@blumamir I had a look and what's happening here is actually related to open-telemetry/opentelemetry-js#4486. In fact, it's that change behaving as expected. 🙂

We're dropping events that happen at 0 now as that suggests falsely suggests that they happen out of order, but what's actually happening is that we don't have any data on them. See entriesFallback: there's unloadEventStart and unloadEventEnd, and they're both 0 and therefore they happen "before" fetchStart and they get dropped.

We'll need to adapt both tests to not expect unloadEventStart and unloadEventEnd for the two failing tests and then we should be good to merge.

JamieDanielson

Thank you for working on this! The updates look great. Also I appreciated the notes on the types that weren't being applied as expected, it was helpful for reviewing.

We'll need to get the test fixed up in a separate PR and then we should be good to go here.

trentm · 2024-04-24T21:21:28Z

We'll need to adapt both tests to not expect unloadEventStart and unloadEventEnd for the two failing tests and then we should be good to merge.

I have a start at a PR for this. Happy for someone else to beat me to it, if you like, though.

trentm · 2024-04-24T21:43:20Z

Please review #2145 to fix the browser-test.

…ion-generic

…into remove-instrumentation-generic

pichlermarc

Thank you for working on this @blumamir. 🎉
Thank you @trentm for fixing the browser tests. 🙂

chore: remove instrumentations generic type to align with upstream

069e709

blumamir requested a review from a team as a code owner April 11, 2024 11:13

github-actions bot assigned blumamir, haddasbronfman, JamieDanielson, martinkuba, maryliag and mottibec Apr 11, 2024

github-actions bot added pkg:instrumentation-ioredis pkg:instrumentation-graphql pkg:instrumentation-mysql labels Apr 11, 2024

github-actions bot assigned naseemkullah Apr 11, 2024

github-actions bot added pkg:instrumentation-mysql2 pkg:instrumentation-fs pkg:instrumentation-restify pkg:instrumentation-dns pkg:instrumentation-lru-memoizer labels Apr 11, 2024

github-actions bot assigned obecny Apr 11, 2024

github-actions bot added pkg:instrumentation-mongoose pkg:instrumentation-tedious pkg:instrumentation-aws-sdk pkg:instrumentation-bunyan pkg:instrumentation-connect pkg:instrumentation-express labels Apr 11, 2024

github-actions bot assigned osherv Apr 11, 2024

github-actions bot added pkg:instrumentation-fastify pkg:instrumentation-generic-pool pkg:instrumentation-hapi pkg:instrumentation-knex pkg:instrumentation-koa labels Apr 11, 2024

blumamir changed the title ~~chore: remove instrumentations generic type to align with upstream~~ chore: experimental 0.51.0, remove instrumentations generic type to align with upstream Apr 24, 2024

blumamir commented Apr 24, 2024

View reviewed changes

fix: apply original types from generics

7016182

blumamir added 3 commits April 24, 2024 20:56

revert: cucumber testing

1ac2cd8

fix: tedious import types

8077112

Merge remote-tracking branch 'upstream/main' into remove-instrumentat…

d96bf87

…ion-generic

JamieDanielson approved these changes Apr 24, 2024

View reviewed changes

trentm mentioned this pull request Apr 24, 2024

test(instr-document-load): fix test to allow missing network span events #2145

Merged

trentm and others added 3 commits April 24, 2024 22:04

Merge branch 'main' into remove-instrumentation-generic

cfde85b

Merge remote-tracking branch 'upstream/main' into remove-instrumentat…

b441107

…ion-generic

Merge remote-tracking branch 'origin/remove-instrumentation-generic' …

8a2a0d3

…into remove-instrumentation-generic

pichlermarc approved these changes Apr 25, 2024

View reviewed changes

pichlermarc merged commit 80cbee7 into open-telemetry:main Apr 25, 2024
18 checks passed

trentm added pkg:instrumentation-cassandra-driver and removed pkg:instrumentation-cassandra labels May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: experimental 0.51.0, remove instrumentations generic type to align with upstream #2091

chore: experimental 0.51.0, remove instrumentations generic type to align with upstream #2091

blumamir commented Apr 11, 2024 •

edited by pichlermarc

Loading

codecov bot commented Apr 24, 2024 •

edited

Loading

blumamir Apr 24, 2024

blumamir Apr 24, 2024

blumamir Apr 24, 2024

blumamir Apr 24, 2024

blumamir Apr 24, 2024

blumamir Apr 24, 2024

blumamir Apr 24, 2024

blumamir Apr 24, 2024

blumamir Apr 24, 2024

blumamir Apr 24, 2024

blumamir commented Apr 24, 2024

blumamir commented Apr 24, 2024

pichlermarc commented Apr 24, 2024

JamieDanielson left a comment

trentm commented Apr 24, 2024

trentm commented Apr 24, 2024

pichlermarc left a comment

		// cast it to module definition of unknown type to avoid exposing Dataloader types on public APIs
		) as InstrumentationNodeModuleDefinition<unknown>,

chore: experimental 0.51.0, remove instrumentations generic type to align with upstream #2091

chore: experimental 0.51.0, remove instrumentations generic type to align with upstream #2091

Conversation

blumamir commented Apr 11, 2024 • edited by pichlermarc Loading

codecov bot commented Apr 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blumamir commented Apr 24, 2024

blumamir commented Apr 24, 2024

pichlermarc commented Apr 24, 2024

JamieDanielson left a comment

Choose a reason for hiding this comment

trentm commented Apr 24, 2024

trentm commented Apr 24, 2024

pichlermarc left a comment

Choose a reason for hiding this comment

blumamir commented Apr 11, 2024 •

edited by pichlermarc

Loading

codecov bot commented Apr 24, 2024 •

edited

Loading