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

fix: remove types of the instrumented libs form public apis #1221

Merged
merged 5 commits into from
Oct 19, 2022

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented Oct 7, 2022

Which problem is this PR solving?

Fixes #1217
Fixes #1218

Short description of the changes

  • Cast Dataloaders module definitions to InstrumentationNodeModuleDefinition<unknown>
  • Turned patch and unpatch methods on mongoose instrumentation private
  • Cast the Collection argument in mongoose untils inside the function as opposed to having the type on the signature

@rauno56
Copy link
Member Author

rauno56 commented Oct 7, 2022

Requesting reviews from the component owners: @henrinormak and @blumamir 🙏

@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Merging #1221 (765ada9) into main (180b336) will increase coverage by 2.64%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1221      +/-   ##
==========================================
+ Coverage   96.08%   98.72%   +2.64%     
==========================================
  Files          14        2      -12     
  Lines         893      157     -736     
  Branches      191       18     -173     
==========================================
- Hits          858      155     -703     
+ Misses         35        2      -33     
Impacted Files Coverage Δ
.../instrumentation-dataloader/src/instrumentation.ts 99.01% <ø> (ø)
...entation-document-load/src/enums/AttributeNames.ts
...trumentation-document-load/src/enums/EventNames.ts
...y-instrumentation-long-task/src/instrumentation.ts
...etry-plugin-react-load/src/enums/AttributeNames.ts
...lugin-react-load/src/BaseOpenTelemetryComponent.ts
...erator-aws-xray/src/internal/xray-id-generation.ts
...emetry-propagator-instana/src/InstanaPropagator.ts
...ation-user-interaction/src/enums/AttributeNames.ts
...etapackages/auto-instrumentations-web/src/utils.ts
... and 7 more

Copy link
Contributor

@henrinormak henrinormak left a comment

Choose a reason for hiding this comment

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

The dataloader part looks good, the mongoose changes syntactically make sense as well, but not super familiar with that package.

Comment on lines 22 to 23
export function getAttributesFromCollection(
collection: Collection
_collection: unknown
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something, but why is this function part of the public api? it is not exported in index.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not exported to index.ts, but it's exported from utils. I expected TS to recursively import ts files and compile them. It seems that I was wrong and in the normal case utils is not compiled. Only when

import * as mongooseUtils from '@opentelemetry/instrumentation-mongoose/build/src/utils';

is run explicitly, the problem occurs. Is there a compiler option that turns that process recursive?

That said, if I understand the situation correctly, we don't have to remove the type from that signature. Thanks for pointing that out, @blumamir!

Copy link
Member

Choose a reason for hiding this comment

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

Is there a compiler option that turns that process recursive?

From what I know, typescript is compiling only the single .d.ts file that is declared in package.json like this:

{
  "types": "index.d.ts"
}

This file in turn can import other type files (or not), but since utils.d.ts is not imported from index.d.ts, then typescript never encounters import * from 'mongoose' so there are no issues.
This is only my understanding, but I could be missing some important details. typescript is so complicated 😅

@blumamir blumamir merged commit 682d610 into open-telemetry:main Oct 19, 2022
@dyladan dyladan mentioned this pull request Oct 19, 2022
@rauno56 rauno56 deleted the fix/type-deps branch October 21, 2022 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants