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: experimental 0.51.0, remove instrumentations generic type to align with upstream #2091

Merged

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented Apr 11, 2024

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

Copy link

codecov bot commented Apr 24, 2024

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 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
Comment on lines -76 to -77
// cast it to module definition of unknown type to avoid exposing Dataloader types on public APIs
) as InstrumentationNodeModuleDefinition<unknown>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

protected init() {
return [
new InstrumentationNodeModuleDefinition<typeof BunyanLogger>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

init() {
return [
new InstrumentationNodeModuleDefinition<any>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -30 to -31
Dns,
DnsPromises,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

private _addPatchingExecute(): InstrumentationNodeModuleFile<
typeof graphqlTypes
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

private getRunnerNodeModuleFileInstrumentation(basePath: string) {
return new InstrumentationNodeModuleFile<typeof knex>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

protected init() {
return new InstrumentationNodeModuleDefinition<typeof koa>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the generic has no effect, moudleExports is typed with any

'mongodb',
['>=3.3 <4'],
undefined,
undefined,
[
new InstrumentationNodeModuleFile<WireProtocolInternal>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch function, v3PatchConnection is already typed correctly

'mongodb',
['4.*', '5.*', '6.*'],
undefined,
undefined,
[
new InstrumentationNodeModuleFile<V4Connection>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch function, v4PatchConnectionCallback is already typed as any

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

protected init() {
const modulePG = new InstrumentationNodeModuleDefinition<typeof pgTypes>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moudle below is already any this type has no effect

@blumamir
Copy link
Member Author

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.

@blumamir
Copy link
Member Author

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

@pichlermarc
Copy link
Member

@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.

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.

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

trentm commented Apr 24, 2024

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

trentm commented Apr 24, 2024

Please review #2145 to fix the browser-test.

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pichlermarc pichlermarc merged commit 80cbee7 into open-telemetry:main Apr 25, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.