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-runtime-node): mixin default config in constructor #2036

Merged

Conversation

satazor
Copy link
Contributor

@satazor satazor commented Mar 24, 2024

Which problem is this PR solving?

The constructor of the instrumentation runtime node does not mixin the default config. This means that if I pass an empty object, the eventLoopUtilizationMeasurementInterval will be undefined, which has the side-effect of the configured interval to be near 0.

Short description of the changes

Similarly to all other instrumentations, I've mixed in the default config to make sure default objects are always defined. Please note that the instrumentation file itself didn't have tests, so I haven't added any.

@satazor
Copy link
Contributor Author

satazor commented Mar 24, 2024

cc @d4nyll @pichlermarc @trentm

Copy link

codecov bot commented Mar 24, 2024

Codecov Report

Merging #2036 (b25230d) into main (dfb2dff) will increase coverage by 0.00%.
Report is 15 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2036   +/-   ##
=======================================
  Coverage   90.97%   90.98%           
=======================================
  Files         146      145    -1     
  Lines        7492     7454   -38     
  Branches     1502     1488   -14     
=======================================
- Hits         6816     6782   -34     
+ Misses        676      672    -4     
Files Coverage Δ
...nstrumentation-runtime-node/src/instrumentation.ts 89.18% <100.00%> (ø)

... and 5 files with indirect coverage changes

Copy link
Member

@d4nyll d4nyll left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @satazor

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.

thanks 👍

@pichlermarc pichlermarc merged commit 146edfa into open-telemetry:main Mar 25, 2024
12 checks passed
@dyladan dyladan mentioned this pull request Mar 25, 2024
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.

3 participants