-
Notifications
You must be signed in to change notification settings - Fork 1
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: Capture request, response headers and body #9
Conversation
ernestii
commented
May 29, 2024
•
edited
Loading
edited
- test on android
- test on ios
17e67de
to
06baa31
Compare
src/instrumentations/xhr.ts
Outdated
const [key, value] = args; | ||
const xhrMem = _xhrMem.get(this); | ||
if (xhrMem && key && value) { | ||
const newKey = `http.request.header.${key.toLowerCase()}`; |
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.
Let's try to make sure the headers extraction matches what otel does in their http instrumentation (https://github.com/open-telemetry/opentelemetry-js/blob/82b7526b028a34a23936016768f37df05effcd59/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts#L583)
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.
updated!
src/instrumentations/xhr.ts
Outdated
'http.request.body', | ||
typeof requestBody === 'string' | ||
? requestBody | ||
: JSON.stringify(requestBody) |
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.
better to handle the case when stringify throws. also I wonder if we want to set a cap on body size ?
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.
added!
src/instrumentations/xhr.ts
Outdated
@@ -753,7 +780,28 @@ export function instrumentXHR(config: XhrConfig) { | |||
} | |||
} | |||
} else if (this.readyState === XMLHttpRequest.DONE) { | |||
endSpan(EventNames.EVENT_READY_STATE_CHANGE, this); | |||
if (config.networkBodyCapture && this.responseType === 'blob') { | |||
try { |
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 what this try-catch is for
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.
probably leftover from experimenting, removed
currentSpan.setAttribute('http.response.body', text); | ||
}) | ||
.finally(() => { | ||
endSpan(EventNames.EVENT_READY_STATE_CHANGE, this); |
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.
just double check. the span would never end before this ?
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 shouldn't
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.
Super excited about this! 🚢 🚢 🚢
src/instrumentations/xhr.ts
Outdated
@@ -47,11 +47,14 @@ import type { PropagateTraceHeaderCorsUrls } from '@opentelemetry/sdk-trace-web/ | |||
|
|||
const parseUrl = (url: string) => new URL(url); | |||
|
|||
const MAX_BODY_LENGTH = 5 * 1024; // 5KB |
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.
5k seems a bit too strict. maybe something like 2MB to start with is okay
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.
updated