-
Notifications
You must be signed in to change notification settings - Fork 819
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
chore: do not inject span context when instrumentation is suppressed #2082
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2082 +/- ##
==========================================
+ Coverage 93.03% 93.07% +0.03%
==========================================
Files 154 154
Lines 5976 5991 +15
Branches 1246 1256 +10
==========================================
+ Hits 5560 5576 +16
+ Misses 416 415 -1
|
packages/opentelemetry-propagator-b3/test/B3MultiPropagator.test.ts
Outdated
Show resolved
Hide resolved
carrier, | ||
defaultTextMapSetter | ||
); | ||
assert.deepStrictEqual(carrier[TRACE_PARENT_HEADER], undefined); |
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.
assert.deepStrictEqual(carrier[TRACE_PARENT_HEADER], undefined); | |
assert.strictEqual(carrier[TRACE_PARENT_HEADER], undefined); |
same on next line
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.
Curious if there is some advantage to using strict over deep strict?
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.
in this usage where one element is undefined
there is most likely no difference. But in other usecases there is:
assert.deepStrictEqual({}, {}) // throws
assert.strictEqual({}, {}) // doesn't throw
At least for me the use of deepXXX
asserts indicates that deep objects should be compared which is not the case here.
But feel free to ignore my comments regarding this is you feel the opposite.
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.
I don't have an opinion at all, was just curious what the reasoning was behind yours.
Fixes #2067