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(sdk-node): update instrumentations once MeterProvider is initialized #3624

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Feb 21, 2023

Which problem is this PR solving?

In @opentelemetry/sdk-node instrumentations are registered before the SDK MeterProvider is registered, causing the instrumentations to be left with a NoopMeterProvider, and subsequently NoopMeter and then no-op instruments, which causes all metrics data provided to these instruments to be dropped.

This is only the case for metrics, as the Trace API implements a delegating no-op instead. Once we implement a delegating no-op for the Metrics API (#3622), the workaround proposed by this PR will become obsolete.

Fixes #3609

Short description of the changes

Updates instrumentations once MeterProvider is registered to avoid no-ops being used.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #3624 (dcb9600) into main (4a4484a) will decrease coverage by 0.71%.
The diff coverage is n/a.

❗ Current head dcb9600 differs from pull request most recent head 1f29c80. Consider uploading reports for the commit 1f29c80 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3624      +/-   ##
==========================================
- Coverage   93.25%   92.54%   -0.71%     
==========================================
  Files         265      172      -93     
  Lines        7484     3929    -3555     
  Branches     1556      796     -760     
==========================================
- Hits         6979     3636    -3343     
+ Misses        505      293     -212     
Impacted Files Coverage Δ
.../packages/api-logs/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 84.74% <0.00%> (-15.26%) ⬇️
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 52.27% <0.00%> (-11.37%) ⬇️
...-trace-base/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...imental/packages/opentelemetry-sdk-node/src/sdk.ts
packages/sdk-metrics/src/view/MeterSelector.ts
...egator/exponential-histogram/mapping/getMapping.ts
...ackages/opentelemetry-shim-opentracing/src/shim.ts
.../packages/otlp-transformer/src/metrics/internal.ts
...y-instrumentation-http/src/enums/AttributeNames.ts
... and 98 more

@pichlermarc pichlermarc marked this pull request as ready for review February 27, 2023 12:14
@pichlermarc pichlermarc requested a review from a team February 27, 2023 12:14
@pichlermarc pichlermarc added the bug Something isn't working label Mar 1, 2023
@dyladan dyladan merged commit cbbdfd3 into open-telemetry:main Mar 2, 2023
@pichlermarc pichlermarc deleted the bugfix/3609-broken-metrics-instrumentations branch March 3, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version 0.35.1 of @opentelemetry/sdk-node broke http instrumentation
3 participants