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): do not wrap preClose and onRequestAbort hooks #1764

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Nov 2, 2023

Fixes: #1762

This changes to an allow-list of hook names to wrap -- which means that if a new hook is added, then we won't create spans for it, but we also won't accidentally crash. :)

@trentm trentm requested a review from a team November 2, 2023 02:45
@github-actions github-actions bot requested a review from pichlermarc November 2, 2023 02:45
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #1764 (da27a09) into main (1b0caa6) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1764   +/-   ##
=======================================
  Coverage   91.44%   91.44%           
=======================================
  Files         143      143           
  Lines        7273     7273           
  Branches     1458     1458           
=======================================
  Hits         6651     6651           
  Misses        622      622           
Files Coverage Δ
...telemetry-instrumentation-fastify/src/constants.ts 100.00% <100.00%> (ø)
...try-instrumentation-fastify/src/instrumentation.ts 96.87% <100.00%> (ø)

Copy link

@SeanReece SeanReece left a comment

Choose a reason for hiding this comment

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

LGTM

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.

This looks great, thank you for taking care of this. 👍
I like the change to make this an allow list instead - makes it much safer in case new hooks are added in fastify.

@pichlermarc pichlermarc merged commit de6156a into open-telemetry:main Nov 6, 2023
15 checks passed
@dyladan dyladan mentioned this pull request Nov 6, 2023
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.

[@opentelemetry/instrumentation-fastify] Breaks use of preClose with fastify
4 participants