-
Notifications
You must be signed in to change notification settings - Fork 846
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(opentelemetry-instrumentation-fetch): added a requestHook option #5380
feat(opentelemetry-instrumentation-fetch): added a requestHook option #5380
Conversation
Signed-off-by: patricklafrance <[email protected]>
… interface Signed-off-by: patricklafrance <[email protected]>
… interface Signed-off-by: patricklafrance <[email protected]>
…nflicts for the fetch.test.ts file Signed-off-by: patricklafrance <[email protected]>
@patricklafrance hey sorry for causing the conflicts in the tests, they have been completely rewritten in #5282. Instead of having one big shared fake-fetch function that we keep adding to to support all the various things we want to test, now each group of test have a smaller and more focused setup block specific to their needs. Hopefully you'd ultimately find that easier to work with, despite the trouble. Without looking into this too deeply, I think the tests for Let me know if you need help working through that. |
… the refactor of the fetch instrumentation in open-telemetry#5282 Signed-off-by: patricklafrance <[email protected]>
Hey @chancancode, I rewrote the tests using the new structure. Because I have to make an assertion on the headers of the fetch request, I added back a stubbed Could you by any chance review this PR and consider merging it? That would be greatly appreciated 🙏🏻 |
…ed packageManager field into the root package.json file Signed-off-by: patricklafrance <[email protected]>
describe('`requesthook` config', () => { | ||
const tracedFetch = async ({ | ||
handlers = [ | ||
msw.http.get('/api/project-headers.json', ({ request }) => { | ||
const headers = new Headers(); | ||
|
||
for (const [key, value] of request.headers) { | ||
headers.set(`x-request-${key}`, value); | ||
} | ||
|
||
return msw.HttpResponse.json({ ok: true }, { headers }); | ||
}), | ||
], | ||
callback = () => fetch('/api/project-headers.json'), | ||
config, | ||
}: { | ||
handlers?: msw.RequestHandler[]; | ||
callback?: () => Promise<Response>; | ||
config: FetchInstrumentationConfig & | ||
Required<Pick<FetchInstrumentationConfig, 'requestHook'>>; | ||
}): Promise<{ rootSpan: api.Span; request: RequestInit }> => { | ||
let request: RequestInit | undefined; | ||
|
||
function fakeFetch( | ||
input: RequestInfo | Request, | ||
init: RequestInit = {} | ||
) { | ||
request = init; | ||
|
||
return new Promise(resolve => { | ||
resolve(new window.Response('', {})); | ||
}); | ||
} | ||
|
||
// Using a stub to get the actual request object that has been tempered by the | ||
// requestHook option. | ||
const fetchStub = sinon | ||
.stub(window, 'fetch') | ||
.callsFake(fakeFetch as any); | ||
|
||
await startWorker(...handlers); | ||
|
||
const rootSpan = await trace(async () => { | ||
await callback(); | ||
}, config); | ||
|
||
fetchStub.reset(); | ||
|
||
assert.strictEqual(exportedSpans.length, 1); | ||
|
||
return { rootSpan, request: request as RequestInit }; | ||
}; | ||
|
||
it('applies attributes to the span', async () => { | ||
await tracedFetch({ | ||
config: { | ||
requestHook: span => { | ||
span.setAttribute('custom.foo', 'bar'); | ||
}, | ||
}, | ||
}); | ||
|
||
const span: tracing.ReadableSpan = exportedSpans[0]; | ||
assert.strictEqual(span.attributes['custom.foo'], 'bar'); | ||
}); | ||
|
||
it('applies headers to the request', async () => { | ||
const { request: traceRequest } = await tracedFetch({ | ||
config: { | ||
requestHook: (span, request) => { | ||
(request.headers as Record<string, string>)['traceparent'] = | ||
'custom-value'; | ||
}, | ||
}, | ||
}); | ||
|
||
assert.strictEqual( | ||
(traceRequest.headers as Record<string, string>)['traceparent'], | ||
'custom-value' | ||
); | ||
}); | ||
}); |
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 am not a maintainer so I can't merge your PR and you should take my suggestions with a grain of salt.
But I don't think it should be necessary to stub fetch in your tests – that's the main issue with the previous tests and what we tried to move away from. It's too easy to mock the browser feature in incorrect/incomplete ways and end up testing behaviors that doesn't exist in the real world, or mask breakages that can only be discovered in production.
Something like this should be sufficient to ensure the feature is working correctly, as I understood it. You should review this carefully to make sure that's the case. I'm also just typing this out in the browser, there may be syntax errors and you may have to make adjustments for TypeScript/eslint/prettier.
describe('`requestHook` config', () => {
const tracedFetch = async ({
handlers = [
msw.http.get('/api/echo-headers.json', ({ request }) => {
return msw.HttpResponse.json({
request: {
headers: Object.fromEntries(request.headers),
},
});
}),
],
callback = () => fetch('/api/echo-headers.json'),
config,
}: {
handlers?: msw.RequestHandler[];
callback?: () => Promise<Response>;
config: FetchInstrumentationConfig &
Required<Pick<FetchInstrumentationConfig, 'requestHook'>>;
}): Promise<{ rootSpan: api.Span; request: RequestInit }> => {
let response: Response | undefined;
await startWorker(...handlers);
const rootSpan = await trace(async () => {
response = await callback();
}, config);
assert.ok(response instanceof Response);
assert.strictEqual(exportedSpans.length, 1);
return { rootSpan, response };
};
it('can apply attributes to the span', async () => {
await tracedFetch({
config: {
requestHook: span => {
span.setAttribute('custom.foo', 'bar');
},
},
});
const span: tracing.ReadableSpan = exportedSpans[0];
assert.strictEqual(span.attributes['custom.foo'], 'bar');
});
it('can remove propagation headers', async () => {
const { response } = await tracedFetch({
config: {
requestHook: (span, request) => {
assert.ok(
request !== null && typeof request === 'object' && !(request instanceof Request),
'`requestHook` should get a `RequestInit` object when no options are passed to `fetch()`'
);
assert.ok(
request.headers !== null && typeof request.headers === 'object',
'`requestHook` should get the `headers` object generated by the instrumentation'
);
assert.ok(
X_B3_TRACE_ID in request.headers,
`trace header '${X_B3_TRACE_ID}' not set`
);
assert.ok(
X_B3_SPAN_ID in request.headers,
`trace header '${X_B3_SPAN_ID}' not set`
);
assert.ok(
X_B3_SAMPLED in request.headers,
`trace header '${X_B3_SAMPLED}' not set`
);
delete request.headers[X_B3_TRACE_ID];
delete request.headers[X_B3_SPAN_ID];
delete request.headers[X_B3_SAMPLED];
},
},
});
await assertNoPropagationHeaders(response);
});
it('can modify headers when called with a string URL', async () => {
const { response } = await tracedFetch({
config: {
requestHook: (span, request) => {
assert.ok(
request !== null && typeof request === 'object' && !(request instanceof Request),
'`requestHook` should get a `RequestInit` object when no options are passed to `fetch()`'
);
request.headers = { 'custom-foo': 'foo' };
},
},
});
const headers = await assertPropagationHeaders(response);
assert.strictEqual(
headers['custom-foo'],
'foo',
'header set from requestHook should be sent',
);
});
it('can modify headers when called with a `Request` object', async () => {
const { response } = await tracedFetch({
config: {
requestHook: (span, request) => {
assert.ok(
request instanceof Request,
'`requestHook` should get the `Request` object passed to `fetch()`'
);
request.headers.set('custom-foo', 'foo');
},
},
callback: () =>
fetch(
new Request('/api/echo-headers.json', {
headers: new Headers({ 'custom-bar: 'bar' }),
})
});
const headers = await assertPropagationHeaders(response);
assert.strictEqual(
headers['custom-foo'],
'foo',
'header set from requestHook should be sent',
);
assert.strictEqual(
headers['custom-bar'],
'foo',
'header set from fetch() should be sent',
);
});
it('can modify headers when called with a `RequestInit` object', async () => {
const { response } = await tracedFetch({
config: {
requestHook: (span, request) => {
assert.ok(
request !== null && typeof request === 'object' && !(request instanceof Request),
'`requestHook` should get the `RequestInit` object passed to `fetch()`'
);
assert.ok(
request.headers !== null && typeof request.headers === 'object',
'`requestHook` should get the `headers` object passed to `fetch()`'
);
(request.headers as Record<string, string>)['custom-foo'] = 'foo';
},
},
callback: () =>
fetch('/api/echo-headers.json', {
headers: { 'custom-bar: 'bar' },
})
});
const headers = await assertPropagationHeaders(response);
assert.strictEqual(
headers['custom-foo'],
'foo',
'header set from requestHook should be sent',
);
assert.strictEqual(
headers['custom-bar'],
'foo',
'header set from fetch() should be sent',
);
});
});
The idea is that we defined a mock api endpoint ("handler" in msw) that simply slurps up any request headers you sent and echo it back as JSON in the response body, so you can make a normal request, have it fully flow through all the real fetch()
function and the network layers in the browser. That way you are testing the real, end-to-end behavior, rather than narrowing mocking just the one thing you expect to happen.
Hope that makes sense.
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.
Smart, thank you, I'll try your suggestion next week 🙏🏻
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.
@patricklafrance glad that was helpful! not sure if you saw but in that gist I added a specific test for overriding (removing) the propagation headers. If that’s the specific use case that motivated you to add this feature, you may want to make sure it’s tested so it doesn’t regress
As an aside, personally I dislike how we are try to preserve the inputs to the It makes the internally instrument code more complicated than it should be, as every place that wants to make modifications to the options has to consider all of those possibilities. But as demonstrated in the tests, it also pushes that complexity down to the consumers, as their hooks also have to be prepared to work with any number of those possibilities. Even if your application's coding style enforces a consistent call-style, one day, you may add a dependency or script that calls If it's up to me, I'd refactor the patched fetch function to always normalize the options into a In any case that's probably out of scope for this PR, so it's not really your problem, and it's not very actionable for you. But this new feature does introduce another spot where this currently not ideal (IMO) semantics leaks out to the consumers that we may have to break if we want to do what I proposed, and I think this is a good opportunity to bring that up for consideration. |
We are trying to integrate Honeycomb tracing to our frontend framework using OTel. Unfortunately, we seem to have hit a limitation with the current API that many others stumble upon as well according to this issue. At the moment, adding this hook seems to be the only way to achieve our goal. We've been staled since last November. |
…questHook option Signed-off-by: patricklafrance <[email protected]>
Thanks @chancancode, I updated the tests following your suggestions, way better! 🙏🏻 |
Signed-off-by: patricklafrance <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5380 +/- ##
=======================================
Coverage 94.77% 94.77%
=======================================
Files 309 309
Lines 7967 7967
Branches 1678 1678
=======================================
Hits 7551 7551
Misses 416 416 |
Signed-off-by: patricklafrance <[email protected]>
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.
Thanks for working on this! I am having some trouble with my local env at the moment in trying to test out potential user agent changes. That is the main open question I have, otherwise this matches the functionality in the http instrumentation so it should be fine.
@@ -54,6 +54,10 @@ export interface FetchCustomAttributeFunction { | |||
): void; | |||
} | |||
|
|||
export interface FetchRequestHookFunction { |
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.
Side note: Reviewing this PR I am realizing we may consider one day separating the interfaces into a separate file like there is for the http instrumentation. Today is not that day!
'`requestHook` should get the `Request` object passed to `fetch()`' | ||
); | ||
|
||
request.headers.set('custom-foo', 'foo'); |
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.
Does this allow overriding of user agent as well? I seem to remember something about not allowing user agent override in the browser, but I could be off on the details.
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.
Hey @JamieDanielson, I added a few assertions suggesting that the User-Agent
header cannot be overridden:
- https://github.com/open-telemetry/opentelemetry-js/pull/5380/files#diff-e410f489fdbd91ab6efe08b360062083668775846340c14272b5c93e6c955f68R1438-R1442
- https://github.com/open-telemetry/opentelemetry-js/pull/5380/files#diff-e410f489fdbd91ab6efe08b360062083668775846340c14272b5c93e6c955f68R1478-R1482
- https://github.com/open-telemetry/opentelemetry-js/pull/5380/files#diff-e410f489fdbd91ab6efe08b360062083668775846340c14272b5c93e6c955f68R1524-R1528
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’m not sure that it is very relevant here. We expose the raw Request/RequestInit object, so you can do whatever the browser would let you do.
I don’t know off the top of my head whether the browser has any restrictions on setting that header, but I dont think we specifically introduced any additional roadblocks/restrictions in our code right? A casual search on SO also seems to suggest the answer may be browser-specific, so testing for that here may be overly specific/brittle IMO.
Also may want to check the casing is correct since the browser normalize the built-in headers.
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 brought up the User Agent scenario specifically because it is a semantic convention and also because I recall differences between backend and frontend permissions with APIs. I remember a while back when trying to update the signal specific headers, I ran into the issue of not being able to alter User Agent and was curious if this was still the case. I think it's useful to have a test asserting behavior of this special header, because if it does break that's an indication of potential unexpected breakage in telemetry as well.
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.
Hmm... I can see where assert.notEqual(...['User-Agent']...)
could be brittle though. I like the idea of asserting the User Agent cannot be overridden, but wonder if a) we should move it into its own separate test block to call it out more explicitly and b) if we can think of another way to test it that isn't notEqual. For example can we get the user agent being defined in the test setup and assert that it has not changed?
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.
Good catch on the assertions!
Yea, it's a strange situation. My quick read on the history is that, at one point, the spec explicitly forbid changing that header. After some lengthy discussion, that ban was dropped, and most browsers followed through, except Chrome (and presumably Edge?) is still holding out due to some unspecified concerns.
I think you also made a good point that we currently set the client UA semconv attribute, and we implicitly assume that it can't be overridden, since we hardcoded the attribute to navigator.userAgent
, which.. now that we discussed this a bunch, is arguably a bug in the browsers that does allow you to override it.
So I think a dedicated test for the currently expected UA header behavior as you suggested, with a comment to investigate further for other browsers, is probably a good idea. We can then punt the work to add Firefox to the roster and adjust the behavior and test when someone gets the time to look into that whole thing?
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 guess that you guys are on a Unix OS right?
I am on Windows and I don't even get any User-Agent
returned, not even useragent
.
Given the following test:
it('cannot override the user agent header', async () => {
const { response } = await tracedFetch({
config: {
requestHook: (span, request) => {
assert.ok(
request instanceof Request,
'`requestHook` should get the `Request` object passed to `fetch()`'
);
request.headers.set('User-Agent', 'foo');
},
},
callback: () =>
fetch(
new Request('/api/echo-headers.json')
),
});
const { request } = await response.json();
assert.notEqual(
request.headers['User-Agent'],
'foo',
'User-Agent should not be overridden'
);
assert.strictEqual(
request.headers['User-Agent'],
DEFAULT_USER_AGENT,
'User-Agent should still be the default user agent'
);
});
request.headers
is {accept: '*/*'}
meaning there's no default user agent value for me and the "overriden" value is not returned, even with a Request
instance.
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.
@JamieDanielson Would you mind merging this PR without adding additional tests related to the user agent? There seems to be many quirks with the user agent header that are kind of related to this PR but also seems to have a larger scope than this PR.
Let me know 🙏🏻
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 I think we should remove all of the user agent tests that were added, as we realized it's not testing what we thought it was testing. Sorry for adding extra work to your plate! I guess we've learned some things along the way 😅 and yes the user agent behavior should probably be investigated and sorted out but is out of scope for this PR.
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 worries @JamieDanielson. I cleaned up the tests, should be good to merge now :)
experimental/packages/opentelemetry-instrumentation-fetch/README.md
Outdated
Show resolved
Hide resolved
…he user-agent header cannot be set within the requestHook option Signed-off-by: patricklafrance <[email protected]>
…ssertions Signed-off-by: patricklafrance <[email protected]>
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.
This looks good, thanks for working on it. Main thing right now is that we are finishing up SDK2.0 and have a feature freeze in place for just over a week longer. I will check in with the other maintainers on timing.
Also if anyone who works more closely with web has thoughts on this please chime in!
Merging this as it looks like the only question left are on release timing. |
Which problem is this PR solving?
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #5084
Add a
requestHook
option to the@opentelemetry/instrumentation-fetch
package.Short description of the changes
Add a
requestHook
option to the@opentelemetry/instrumentation-fetch
package.Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added the following unit tests to the
@opentelemetry/instrumentation-fetch
package:The tests can be executed by following those instructions:
cd experimental/packages/opentelemetry-instrumentation-fetch npm run test:browser
Checklist: