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(instrumentation-amqplib): move @types/amqplib into dev deps #1320

Merged
merged 12 commits into from
Feb 7, 2023

Conversation

AbhiPrasad
Copy link
Member

Which problem is this PR solving?

Don't include @types/amqplib in the final bundle with the amqplib instrumentation. This can cause issues if you are using an old typescript version (as you can type checks the deps of @types/amqplib).

Short description of the changes

Move @types/amqplib into devDependencies from dependencies

@AbhiPrasad AbhiPrasad requested a review from a team December 6, 2022 13:40
@github-actions github-actions bot requested a review from blumamir December 6, 2022 13:41
@Flarna
Copy link
Member

Flarna commented Dec 6, 2022

This is problematic because the types from amqplib are used in public types.

Therefore this would result in build problems for users of e.g. @opentelemetry/auto-instrumentations-node if they have these types not installed.

Therefore the exposed API of this instrumentation needs to be changed to no longer use these types.

@osherv
Copy link
Member

osherv commented Dec 19, 2022

So that means, you need to create a new internal-types.ts that includes the types, as implemented here.
also, in some places, you probably can change the signature of the function arg types to be any.
You can see an example here.

@blumamir
Copy link
Member

blumamir commented Jan 1, 2023

@AbhiPrasad Thanks for opening this PR. As mentioned in the comments, the suggested fix is currently problematic. Are you still interested in finalizing it?
@osherv described a possible fix that will not expose the package types in public APIs, and you can also further read here

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #1320 (8353d80) into main (ad92673) will decrease coverage by 1.03%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1320      +/-   ##
==========================================
- Coverage   96.08%   95.05%   -1.03%     
==========================================
  Files          14       18       +4     
  Lines         893     1275     +382     
  Branches      191      280      +89     
==========================================
+ Hits          858     1212     +354     
- Misses         35       63      +28     
Impacted Files Coverage Δ
plugins/node/instrumentation-amqplib/src/types.ts 100.00% <ø> (ø)
...lugins/node/instrumentation-amqplib/src/amqplib.ts 90.40% <100.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 98.21% <0.00%> (ø)
plugins/node/instrumentation-amqplib/src/utils.ts 95.38% <0.00%> (ø)

@AbhiPrasad
Copy link
Member Author

Hey folks - sorry for the delay, was on vacation the past month.

As mentioned in the comments, the suggested fix is currently problematic. Are you still interested in finalizing it?

Yes I am! Will update the PR as soon as I can.

@AbhiPrasad
Copy link
Member Author

Created a internal-types.ts and vendored in the appropriate types.

@AbhiPrasad
Copy link
Member Author

@blumamir Thank you for the review. I made the changes as requested. Ended up removing internal types as it wasn't needed, everything needs to be exposed. Added notes around where I vendored the types from, as well as the version + git sha and included links.

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

Thanks!
LGTM, added 2 minor comments and then we can merge :)

plugins/node/instrumentation-amqplib/src/internal-types.ts Outdated Show resolved Hide resolved
plugins/node/instrumentation-amqplib/src/types.ts Outdated Show resolved Hide resolved
@AbhiPrasad
Copy link
Member Author

Thanks for all the help @blumamir - appreciate it!

@AbhiPrasad
Copy link
Member Author

Hey? Is there a timeline to get this merged and released? Anything I can do to help here?

@blumamir
Copy link
Member

blumamir commented Feb 7, 2023

Sorry about the delay. updated the branch and will merge when CI finish.

Thanks again for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants