Skip to content

fix(opentelemetry-instrumentation-fetch): fixed override of headers #2426

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.

Sign up for GitHub

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

Merged
dyladan merged 5 commits into from
Aug 27, 2021
Merged

fix(opentelemetry-instrumentation-fetch): fixed override of headers #2426

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c6274ef
fix(opentelemetry-instrumentation-fetch): fixed override of headers
philipszalla Aug 23, 2021
db113d1
Merge branch 'main' into fix/instrumentation-fetch-headers
philipszalla Aug 27, 2021
ad890e9
style(opentelemetry-instrumentation-fetch): removed irritating comment
philipszalla Aug 27, 2021
9f49361
test(opentelemetry-instrumentation-fetch): added tests for custom hea…
philipszalla Aug 27, 2021
ab31805
Merge branch 'main' into fix/instrumentation-fetch-headers
dyladan Aug 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/opentelemetry-instrumentation-fetch/src/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,11 @@ export class FetchInstrumentation extends InstrumentationBase<
api.propagation.inject(api.context.active(), options.headers, {
set: (h, k, v) => h.set(k, typeof v === 'string' ? v : String(v)),
});
} else if(options.headers instanceof Headers) {
// ||-operator causes ts type error
Copy link
Member

@dyladan dyladan Aug 24, 2021

Choose a reason for hiding this comment

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

What is this comment referring to?

Copy link
Contributor Author

@philipszalla philipszalla Aug 24, 2021

Choose a reason for hiding this comment

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

I tried to use the ||-operator inside the if-statement to avoid the else if and the code duplication. But it caused a type error at the setter-parameter of the api.propagation.inject-function.

Copy link
Member

@dyladan dyladan Aug 26, 2021

Choose a reason for hiding this comment

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

The comment is pretty unclear on its own. I would just remove it since it's a typing issue and not a functionality caveat.

api.propagation.inject(api.context.active(), options.headers, {
set: (h, k, v) => h.set(k, typeof v === 'string' ? v : String(v)),
});
} else {
const headers: Partial<Record<string, unknown>> = {};
api.propagation.inject(api.context.active(), headers);
Expand Down