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-fastify): stop using fastify types in public api #1267

Merged
merged 1 commit into from
Nov 3, 2022
Merged

fix(instrumentation-fastify): stop using fastify types in public api #1267

merged 1 commit into from
Nov 3, 2022

Conversation

dotboris
Copy link
Contributor

@dotboris dotboris commented Nov 3, 2022

Which problem is this PR solving?

Related to #1266

Prior to this fix, typescript users that have the fastify instrumentation installed but do not have the fastify package installed would get compilation errors because typescript was expecting the fastify package to be present. This issue is bound to happen for anyone using the auto instrumentation feature.

Short description of the changes

  • Mark all methods that use types from fastify as private in instrumentation.ts.

@dotboris dotboris requested a review from a team November 3, 2022 13:15
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: dotboris / name: Boris Bera (499e4d7)

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #1267 (863b62c) into main (ae76570) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1267      +/-   ##
==========================================
- Coverage   96.08%   96.06%   -0.02%     
==========================================
  Files          14       19       +5     
  Lines         893     1117     +224     
  Branches      191      230      +39     
==========================================
+ Hits          858     1073     +215     
- Misses         35       44       +9     
Impacted Files Coverage Δ
...try-instrumentation-fastify/src/instrumentation.ts 97.41% <100.00%> (ø)
...telemetry-instrumentation-fastify/src/constants.ts 100.00% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 98.18% <0.00%> (ø)
...opentelemetry-instrumentation-fastify/src/utils.ts 87.50% <0.00%> (ø)
...nstrumentation-fastify/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)

@dotboris dotboris requested a review from blumamir November 3, 2022 14:11
Prior to this commit, the Fastify instrumentation used to expose a few
types from the `fastify` lib directly.

Exposing these types leads to issues for TypeScript users who depend on
the integration but don't depend on `fastify` itself where their TypeScript
compilation would fail while complaining that it couldn't find any definitions
for `fastify`.

This scenario is pretty common when using the auto-instrumentation
packages.5

Signed-off-by: Boris Bera <[email protected]>
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 for submitting a fix

@dyladan dyladan merged commit 40515c3 into open-telemetry:main Nov 3, 2022
@dyladan dyladan mentioned this pull request Nov 3, 2022
@dotboris dotboris deleted the fix-fastify-type-imports branch November 3, 2022 19:32
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.

3 participants