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

feat(instrumentation-xhr): add applyCustomAttributesOnSpan hook #2134

Merged
merged 10 commits into from
Apr 25, 2021

Conversation

mhennoch
Copy link
Contributor

We've had couple of asks to add custom attributes to already autoinstrumented XHR. It seems fetch already has this functionality #2071. There is a little annoyance that I can't make the hook function parameters the same as there is no separate Request and Response available in XHR. So the hook has span and xhr reference.

Added tests for different fail types(maybe overkill).

@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #2134 (e4c8e33) into main (f077df3) will increase coverage by 0.03%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main    #2134      +/-   ##
==========================================
+ Coverage   92.76%   92.79%   +0.03%     
==========================================
  Files         140      140              
  Lines        4987     4998      +11     
  Branches     1029     1032       +3     
==========================================
+ Hits         4626     4638      +12     
+ Misses        361      360       -1     
Impacted Files Coverage Δ
...emetry-instrumentation-xml-http-request/src/xhr.ts 96.58% <90.90%> (-0.33%) ⬇️
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts 88.75% <0.00%> (+1.25%) ⬆️
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

mhennoch and others added 2 commits April 21, 2021 17:52
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
@dyladan dyladan added the enhancement New feature or request label Apr 21, 2021
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, the additional level with describe looks nice on html report when they are joined with some AND / OR dependency - this is just suggestion not a blocker. Thx for more tests :)

mhennoch and others added 5 commits April 22, 2021 20:30
…hr.test.ts

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
…hr.test.ts

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
…hr.test.ts

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
…hr.test.ts

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
@vmarchaud vmarchaud merged commit d504f32 into open-telemetry:main Apr 25, 2021
@dyladan dyladan mentioned this pull request Jun 2, 2021
@CAaRrLl
Copy link

CAaRrLl commented Mar 25, 2022

We've had couple of asks to add custom attributes to already autoinstrumented XHR. It seems fetch already has this functionality #2071. There is a little annoyance that I can't make the hook function parameters the same as there is no separate Request and Response available in XHR. So the hook has span and xhr reference.

I want to add the request body to span attributes, but the xhr reference can not seem to get the request body. Is there a good solution for it? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants