-
Notifications
You must be signed in to change notification settings - Fork 574
feat(instrumentation-nestjs-core): add support for NestJS 11 #2685
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
feat(instrumentation-nestjs-core): add support for NestJS 11 #2685
Conversation
|
6747378
to
9f42c15
Compare
9f42c15
to
2cc7772
Compare
This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. |
06d3115
to
09f29c9
Compare
@dyladan any chance to have some review, or any advice to move forward with this instrumentation |
Thanks for your contribution @neilime :) you need to also update |
e945812
to
62582a7
Compare
@david-luna it's done |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2685 +/- ##
=======================================
Coverage 90.55% 90.55%
=======================================
Files 168 168
Lines 8078 8078
Branches 1548 1548
=======================================
Hits 7315 7315
Misses 763 763
🚀 New features to boost your workflow:
|
2825793
to
ff88bc2
Compare
@david-luna or @pichlermarc can you approve the workflows, I've fixed the failing tests. Thanks |
Unit tests are failing for Node.js v14 > @opentelemetry/instrumentation-nestjs-core@0.44.0 test
> nyc mocha --timeout 5000 'test/**/*.test.ts'
nestjs-core
1) "before each" hook for "should capture setup"
2) "after each" hook for "should capture setup"
0 passing (58ms)
2 failing
1) nestjs-core
"before each" hook for "should capture setup":
/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-nestjs-core/node_modules/@nestjs/common/file-stream/streamable-file.js:36
this.options.length ??= bufferOrReadStream.length;
^^^ That version of node does not support this kind of assignment. We should skip tests for that version of node. |
That's weird, as the test is failing with "@nestjs/common@^8.0.0", so something that didnt change. I will test them one by one in nodejs 14 context |
ff88bc2
to
29ec165
Compare
@david-luna I will separate the PR in 2 commits, one for dev deps upgrade, one for NestJS 11 upgrade. I've made the first one, it works fine on local with nodejs 14, can you approve workflow to validate it passes on CI |
@neilime I think this line should be updated too: |
Yes it will be but as we have some weird issue with node 14, I separate the PR in two commits: See #2685 (comment) I'm waiting the CI validation of the first one before doing the second one |
Now it's failing on mongodb tests with nodejs 18... Looks more and more weird. I will try to rebase to be up to date with main. Is there any way to allows workflow every time for this PR? it is very time consuming to wait for validation every time. |
29ec165
to
1190482
Compare
@david-luna @gperdomor @pichlermarc, I think the PR is good to pass all tests. After many attempts to fix flaky tests, handle node 14 issues... I've made the CI pass on my form repository, I you can approve the workflow it will be great. Thanks |
Hello, is there a way to help on this PR ? |
On my side, I'm just waiting for CI workflows approval and some reviews |
1190482
to
e910bfd
Compare
Thanks a million for this @neilime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
It seems the modification is very simple, that CI checks are ok (tests and code quality), so this PR can be approved
e910bfd
to
e6f04e8
Compare
@david-luna it's done, thank you for checking it. |
plugins/node/opentelemetry-instrumentation-nestjs-core/package.json
Outdated
Show resolved
Hide resolved
e6f04e8
to
4e0613b
Compare
package-lock.json
Outdated
@@ -35664,31 +35290,6 @@ | |||
"@opentelemetry/api": "^1.3.0" | |||
} | |||
}, | |||
"plugins/node/opentelemetry-instrumentation-aws-sdk/node_modules/@aws-sdk/types": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd and unrelated to the dependencies we're modifying in this PR. Could you please sync with main and run npm install
again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-luna done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-luna to be clean I've restarted from main and re-apply needed changes. Now the package-lock.json must be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! Thank you :)
4e0613b
to
5863c2b
Compare
Signed-off-by: Emilien Escalle <emilien.escalle@escemi.com>
f3ac768
to
b1fbaf7
Compare
The only error in CI is the |
…lemetry#2685) Signed-off-by: Emilien Escalle <emilien.escalle@escemi.com>
Which problem is this PR solving?
Short description of the changes