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-pino): instrument pino used in ESM #1831

Merged
merged 6 commits into from
Dec 7, 2023

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Nov 27, 2023

This also reduces the number of pino versions tested with
test-all-versions test (from 42 to 13, currently).

Fixes: #1587

This also reduces the number of pino versions tested with
test-all-versions test (from 42 to 13, currently).
@trentm trentm self-assigned this Nov 27, 2023
@trentm trentm requested a review from a team November 27, 2023 20:18
@@ -1,5 +1,5 @@
pino:
- versions: "^8.0.0 || ^7.11.0 || 7.8.0 || 7.2.0 || ^6.13.1 || 5.17.0 || 5.14.0"
- versions: "^8.16.2 || 8.12.1 || 8.8.0 || 8.4.0 || 8.0.0 || ^7.11.0 || 7.8.0 || 7.2.0 || ^6.13.3 || 5.17.0 || 5.14.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: This reduces the number of tested versions from 42 to 13

Before:

{"name":"pino","semver":"^8.0.0 || ^7.11.0 || 7.8.0 || 7.2.0 || ^6.13.1 || 5.17.0 || 5.14.0","cmds":["npm run test"],"versions":["5.14.0","5.17.0","6.13.1","6.13.2","6.13.3","7.2.0","6.13.4","7.8.0","6.14.0","7.11.0","8.0.0","8.1.0","8.2.0","8.3.0","8.3.1","8.4.0","8.4.1","8.4.2","8.5.0","8.6.0","8.6.1","8.7.0","8.8.0","8.9.0","8.10.0","8.11.0","8.12.0","8.12.1","8.14.0","8.14.1","8.14.2","8.15.0","8.15.1","8.15.2","8.15.3","8.15.4","8.15.5","8.15.6","8.15.7","8.16.0","8.16.1","8.16.2"],"numVersions":42}

after:

{"name":"pino","semver":"^8.16.2 || 8.12.1 || 8.8.0 || 8.4.0 || 8.0.0 || ^7.11.0 || 7.8.0 || 7.2.0 || ^6.13.3 || 5.17.0 || 5.14.0","cmds":["npm run test"],"versions":["5.14.0","5.17.0","6.13.3","7.2.0","6.13.4","7.8.0","6.14.0","7.11.0","8.0.0","8.4.0","8.8.0","8.12.1","8.16.2"],"numVersions":13}

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Merging #1831 (b10ec11) into main (8f2a195) will decrease coverage by 0.04%.
The diff coverage is 70.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1831      +/-   ##
==========================================
- Coverage   91.49%   91.46%   -0.04%     
==========================================
  Files         144      144              
  Lines        7388     7394       +6     
  Branches     1474     1477       +3     
==========================================
+ Hits         6760     6763       +3     
- Misses        628      631       +3     
Files Coverage Δ
...emetry-instrumentation-pino/src/instrumentation.ts 95.89% <70.00%> (-4.11%) ⬇️

if (isESM) {
module.pino = patchedPino;
module.default = patchedPino;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Codecov / codecov/patch
plugins/node/opentelemetry-instrumentation-pino/src/instrumentation.ts#L82-L83
Added lines #L82 - L83 were not covered by tests

These lines actually are covered by the test, but I guess the out of process runTestFixture doesn't catch that.

@trentm trentm requested a review from JamieDanielson December 6, 2023 16:31
@trentm
Copy link
Contributor Author

trentm commented Dec 6, 2023

The pino 5.x part of the TAV tests now look like this:

...
-- required packages ["pino@5.14.0"]
-- installing ["pino@5.14.0"]
-- running test "npm run test" with pino (env: {})

> @opentelemetry/instrumentation-pino@0.34.3 test
> nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'

  PinoInstrumentation
    ✓ should work with ESM default import (513ms)
    - should work with ESM named import
    enabled instrumentation
...

It was good to do this extra work, because it helped me find that the actual pino version where the named-ESM-exports thing changed was v6.8.0, not right at v6.0.0.

@trentm
Copy link
Contributor Author

trentm commented Dec 6, 2023

@JamieDanielson This is ready for re-review if/when you have a chance. Thanks!

Copy link
Member

@JamieDanielson JamieDanielson 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 fixing this!

@trentm trentm enabled auto-merge (squash) December 7, 2023 00:10
@trentm trentm merged commit 4782f5b into open-telemetry:main Dec 7, 2023
6 checks passed
@trentm trentm deleted the tm-pino-esm branch December 7, 2023 00:10
@dyladan dyladan mentioned this pull request Dec 7, 2023
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.

Pino instrumentation fails when using ES modules
2 participants