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): use TestMetricReader, unref timer #1965

Merged

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Feb 27, 2024

Which problem is this PR solving?

  • tests were failing as the utilization ended up being 1 (fix(instr-perf-hooks): avoid 'prepare' npm script because that runs on 'npm install' #1955) . This might have happened as the collection happened at a time where event loop was fully utilized. This PR attempts to fix this by doing two things:
    • changing the PeriodicExportingMetricReader to avoid having to wait for collection, getting rid of one variable that might break tests
    • allowing <=1 as a value as it's a valid value for a fully utilized event loop (which can happen in some cases as collection does not need to wait for anything that's not doing work on the event loop).
  • for me the process kept running as disable() was never called when the test failed. I can see this happening in apps too as it's rather uncommon to disable() instrumentations before shutting down. Therefore in unrefed the timer when it's created.

Fixes #1960

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Merging #1965 (bc77574) into main (ff40cc8) will decrease coverage by 0.06%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head bc77574 differs from pull request most recent head 07855ba. Consider uploading reports for the commit 07855ba to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1965      +/-   ##
==========================================
- Coverage   91.03%   90.97%   -0.06%     
==========================================
  Files         146      146              
  Lines        7492     7492              
  Branches     1501     1502       +1     
==========================================
- Hits         6820     6816       -4     
- Misses        672      676       +4     
Files Coverage Δ
...nstrumentation-runtime-node/src/instrumentation.ts 89.18% <100.00%> (-10.82%) ⬇️

@pichlermarc pichlermarc marked this pull request as ready for review February 27, 2024 13:29
@pichlermarc pichlermarc requested a review from a team as a code owner February 27, 2024 13:29
@d4nyll
Copy link
Contributor

d4nyll commented Mar 4, 2024

replacing interval-based collection with on-demand collection

@pichlermarc thanks for cleaning up these tests, they look much sturdier now

unref

Good call 👍

@pichlermarc pichlermarc changed the title fix(instrumentation-perf-hooks): use TestMetricReader, unref timer fix(instrumentation-runtime-node): use TestMetricReader, unref timer Mar 5, 2024
await new Promise(resolve => setTimeout(resolve, MEASUREMENT_INTERVAL * 5));
const { resourceMetrics, errors } = await metricReader.collect();

// assert
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL: the arrange-act-assert pattern name (http://wiki.c2.com/?ArrangeActAssert)

@pichlermarc pichlermarc merged commit e24797e into open-telemetry:main Mar 7, 2024
12 checks passed
@dyladan dyladan mentioned this pull request Mar 7, 2024
@pichlermarc pichlermarc deleted the fix/perf-hooks-tests branch May 2, 2024 12:01
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.

flaky instrumentation-runtime-node test
3 participants