-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(node): Add tracing without performance to Node Undici #8449
Conversation
I have no idea why these are failing - the tests all pass for me locally :/ |
d1856cc
to
642001a
Compare
size-limit report 📦
|
9e6c6ef
to
fb3ff50
Compare
The bug with Undici tests ended up being an issue with multiple test server using the same port I'm going to merge this in for now, but I have a todo to come in and clean up the tests here. |
c95d42b
to
3427795
Compare
6166a32
to
3fea09d
Compare
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.
LGTM!
@@ -51,10 +51,10 @@ conditionalTest({ min: 16 })('Undici integration', () => { | |||
it.each([ | |||
[ | |||
'simple url', | |||
'http://localhost:18099', | |||
'http://localhost:18100', |
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.
no action required: Any particular reason for the port change?
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.
Yes - this was because 18099
was used by another test so running the tests in parallel would lead to issues. See: #8449 (comment)
ref #8352
Updates the Node Undici integration to always attach
sentry-trace
headers to outgoing requests.This can be controlled with the top level
tracePropagationOptions
option.