-
Notifications
You must be signed in to change notification settings - Fork 821
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-fetch): Add applyCustomAttributesOnSpan option #2071
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2071 +/- ##
==========================================
+ Coverage 92.14% 92.73% +0.59%
==========================================
Files 120 137 +17
Lines 3958 4915 +957
Branches 809 1015 +206
==========================================
+ Hits 3647 4558 +911
- Misses 311 357 +46
|
@@ -329,46 +336,51 @@ describe('fetch', () => { | |||
const keys = Object.keys(attributes); | |||
|
|||
assert.ok( | |||
attributes[keys[0]] !== '', | |||
attributes[AttributeNames.COMPONENT] !== '', |
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 want to modify existing tests too much but the order of attributes changed so perhaps using attribute keys is more accurate?
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.
Is it possible to use assert.deepStrictEqual?
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.
It's going to be tricky because of the http.user_agent
and http.host
attribute depend on the context of the test runner I believe thats why these attributes are not checked for equality.
I could make achange to use deepStrictEqual
for span attributes that do not depend on environment though, wdyt?
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've changed it to assert useragent and host separately and use deepStrictEqual
for the other properties 🙏
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.
Reverted the changes to put everything in a new decribe block
packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
Outdated
Show resolved
Hide resolved
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, one final suggestion
This PR adds an
applyCustomAttributesOnSpan
function on theinstrumentation-fetch
package. similar to the API of http(s) plugins.Fixes #1287
Happy to make changes in tests or implementation if needed