-
Notifications
You must be signed in to change notification settings - Fork 838
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
draft: fetch instrumentation header attributes #5354
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -32,6 +32,8 @@ import { | |||||
SEMATTRS_HTTP_URL, | ||||||
SEMATTRS_HTTP_METHOD, | ||||||
SEMATTRS_HTTP_REQUEST_CONTENT_LENGTH_UNCOMPRESSED, | ||||||
ATTR_HTTP_REQUEST_HEADER, | ||||||
ATTR_HTTP_RESPONSE_HEADER, | ||||||
} from '@opentelemetry/semantic-conventions'; | ||||||
import { FetchError, FetchResponse, SpanData } from './types'; | ||||||
import { getFetchBodyLength } from './utils'; | ||||||
|
@@ -58,26 +60,37 @@ export interface FetchCustomAttributeFunction { | |||||
* FetchPlugin Config | ||||||
*/ | ||||||
export interface FetchInstrumentationConfig extends InstrumentationConfig { | ||||||
// the number of timing resources is limited, after the limit | ||||||
// (chrome 250, safari 150) the information is not collected anymore | ||||||
// the only way to prevent that is to regularly clean the resources | ||||||
// whenever it is possible, this is needed only when PerformanceObserver | ||||||
// is not available | ||||||
/** Function for adding custom attributes on the span */ | ||||||
applyCustomAttributesOnSpan?: FetchCustomAttributeFunction; | ||||||
|
||||||
/** the number of timing resources is limited, after the limit | ||||||
(chrome 250, safari 150) the information is not collected anymore | ||||||
the only way to prevent that is to regularly clean the resources | ||||||
whenever it is possible, this is needed only when PerformanceObserver | ||||||
is not available */ | ||||||
clearTimingResources?: boolean; | ||||||
// urls which should include trace headers when origin doesn't match | ||||||
propagateTraceHeaderCorsUrls?: web.PropagateTraceHeaderCorsUrls; | ||||||
|
||||||
/** List of request headers to include as attributes on the span. */ | ||||||
requestHeadersAsAttributes?: string[]; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: is it on the roadmap to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not proposing that we consider support for declarative config here, but should these properties try to match naming for properties defined in I believe |
||||||
|
||||||
/** List of request headers to include as attributes on the span. */ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
responseHeadersAsAttributes?: string[]; | ||||||
|
||||||
/** Ignore adding network events as span events */ | ||||||
ignoreNetworkEvents?: boolean; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Is there any hard dependency of |
||||||
|
||||||
/** | ||||||
* URLs that partially match any regex in ignoreUrls will not be traced. | ||||||
* In addition, URLs that are _exact matches_ of strings in ignoreUrls will | ||||||
* also not be traced. | ||||||
*/ | ||||||
ignoreUrls?: Array<string | RegExp>; | ||||||
/** Function for adding custom attributes on the span */ | ||||||
applyCustomAttributesOnSpan?: FetchCustomAttributeFunction; | ||||||
// Ignore adding network events as span events | ||||||
ignoreNetworkEvents?: boolean; | ||||||
|
||||||
/** Measure outgoing request size */ | ||||||
measureRequestSize?: boolean; | ||||||
|
||||||
/** urls which should include trace headers when origin doesn't match */ | ||||||
propagateTraceHeaderCorsUrls?: web.PropagateTraceHeaderCorsUrls; | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -343,8 +356,20 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati | |||||
}); | ||||||
} | ||||||
|
||||||
const requestHeadersToIncludeAsAttributes = | ||||||
plugin.getConfig().requestHeadersAsAttributes; | ||||||
const responseHeadersToIncludeAsAttributes = | ||||||
plugin.getConfig().responseHeadersAsAttributes; | ||||||
|
||||||
function endSpanOnError(span: api.Span, error: FetchError) { | ||||||
plugin._applyAttributesAfterFetch(span, options, error); | ||||||
if (requestHeadersToIncludeAsAttributes) { | ||||||
plugin._applyRequestHeadersAsAttributes( | ||||||
span, | ||||||
requestHeadersToIncludeAsAttributes, | ||||||
options.headers as Headers | ||||||
); | ||||||
} | ||||||
plugin._endSpan(span, spanData, { | ||||||
status: error.status || 0, | ||||||
statusText: error.message, | ||||||
|
@@ -354,6 +379,22 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati | |||||
|
||||||
function endSpanOnSuccess(span: api.Span, response: Response) { | ||||||
plugin._applyAttributesAfterFetch(span, options, response); | ||||||
if (requestHeadersToIncludeAsAttributes) { | ||||||
plugin._applyRequestHeadersAsAttributes( | ||||||
span, | ||||||
requestHeadersToIncludeAsAttributes, | ||||||
options.headers as Headers | ||||||
); | ||||||
} | ||||||
|
||||||
if (responseHeadersToIncludeAsAttributes) { | ||||||
plugin._applyResponseHeadersAsAttributes( | ||||||
span, | ||||||
responseHeadersToIncludeAsAttributes, | ||||||
response.headers | ||||||
); | ||||||
} | ||||||
|
||||||
if (response.status >= 200 && response.status < 400) { | ||||||
plugin._endSpan(span, spanData, response); | ||||||
} else { | ||||||
|
@@ -435,6 +476,32 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati | |||||
}; | ||||||
} | ||||||
|
||||||
private _applyRequestHeadersAsAttributes( | ||||||
span: api.Span, | ||||||
headersToInclude: string[], | ||||||
requestHeaders: Headers | ||||||
) { | ||||||
headersToInclude.forEach(header => { | ||||||
const value = requestHeaders.get(header); | ||||||
if (value) { | ||||||
span.setAttribute(ATTR_HTTP_REQUEST_HEADER(header), value); | ||||||
} | ||||||
}); | ||||||
} | ||||||
|
||||||
private _applyResponseHeadersAsAttributes( | ||||||
span: api.Span, | ||||||
headersToInclude: string[], | ||||||
responseHeaders: Headers | ||||||
) { | ||||||
headersToInclude.forEach(header => { | ||||||
const value = responseHeaders.get(header); | ||||||
if (value) { | ||||||
span.setAttribute(ATTR_HTTP_RESPONSE_HEADER(header), value); | ||||||
} | ||||||
}); | ||||||
} | ||||||
|
||||||
private _applyAttributesAfterFetch( | ||||||
span: api.Span, | ||||||
request: Request | RequestInit, | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,7 @@ import { | |
SEMATTRS_HTTP_URL, | ||
SEMATTRS_HTTP_USER_AGENT, | ||
SEMATTRS_HTTP_REQUEST_CONTENT_LENGTH_UNCOMPRESSED, | ||
ATTR_HTTP_REQUEST_HEADER, | ||
} from '@opentelemetry/semantic-conventions'; | ||
|
||
class DummySpanExporter implements tracing.SpanExporter { | ||
|
@@ -1222,4 +1223,22 @@ describe('fetch', () => { | |
); | ||
}); | ||
}); | ||
|
||
describe('when request headers are applied as attributes', () => { | ||
afterEach(() => { | ||
clearData(); | ||
}); | ||
|
||
it('applies request headers supplied in config', async () => { | ||
await prepareData(url, () => getData(url), { | ||
requestHeadersAsAttributes: ['foo', 'Content-Type'], | ||
}); | ||
const span: tracing.ReadableSpan = exportSpy.args[1][0][0]; | ||
assert.ok(span.attributes[ATTR_HTTP_REQUEST_HEADER('foo')] === 'bar'); | ||
assert.ok( | ||
span.attributes[ATTR_HTTP_REQUEST_HEADER('Content-Type')] === | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The spec expects "normalized HTTP Header name (lowercase)" so this test is a bit misleading/wrong |
||
'application/json' | ||
); | ||
}); | ||
}); | ||
}); |
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 rearranging these config options into alphabetical order.