draft: fetch instrumentation header attributes

[pkanal]

Which problem is this PR solving?

This PR adds options to add headers as attributes for the browser request fetch instrumentation. The http.request.header. and http.response.header. are stable semantic attributes so we should have support for them in the fetch instrumentation.

Short description of the changes

• Added requestHeadersAsAttributes as a config param, an allowlist of request headers that should be included as attributes. By default no headers are added as attributes so that we avoid any security concerns.
• Added responseHeadersAsAttributes as a config param, an allowlist of response headers that should be included as attributes.

Documentation updates to come once we feel like the config API makes sense.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Unit tests.

Checklist:

• [ x] Followed the style guidelines of this project
• [ x] Unit tests have been added
• Documentation has been updated

[pkanal]

Just rearranging these config options into alphabetical order.

[williazz]

Suggested change

	/** List of request headers to include as attributes on the span. */
	/** List of response headers to include as attributes on the span. */

[williazz]

Question: Is there any hard dependency of requestHeadersAsAttributes/responseHeadersAsAttributes on ignoreNetworkEvents/propagateTraceHeaderCorsUrls/applyCustomAttributesOnSpan? If not, maybe they can be separated them into two PRs.

[williazz]

Q: is it on the roadmap to add requestHeadersAsAttributes/responseHeadersAsAttributes to Xhr instrumentation?

[williazz]

Specifically give consideration to naming of config options (thanks!)

I think the two new config names requestHeadersAsAtributes and responseHeadersAsAttributes are wise.

Anything else would be kind of awkward. For example

• applyRequestHeadersAsAttributesOnSpan
• applyReqHeadersAsAttrOnSpan

[danielgblanco]

Not proposing that we consider support for declarative config here, but should these properties try to match naming for properties defined in opentelemetry-configuration, like HttpInstrumentation which will ultimately get support following that schema?

I believe instrumentation.general.http.client.request_captured_headers would be the corresponding property here. Would something like requestCapturedHeaders make it clearer fo folks that want to move to file config in the future?

[chancancode]

A couple of general notes:

• Personally, I would love to see [instrumentation-fetch] Use msw for fetch instrumentation tests #5282 merged before more changes are landed.

• We should probably strive to have some feature parity between fetch and xhr instrumentation?

• Some of the code as-written in the draft is unsafe.

  In particular, I don't think the cast to Headers is safe. We take whatever inputs the user pass to fetch() and generally try to preserve it without normalization. So there could the Headers object like you expected, but also possibly no headers, it could be a POJO, could even be a Map. To be honest I wish we'd just always normalize the inputs at the top into the canonical fetch(RequestObject) form, but without that refactor you'd have to be more defensive about handling all the different possibilities.

  Likewise, on the response side, you'd have to account for the possibilities of an opaque response, though it may be fine as is if it just allows you to call those methods but just returns nothing, without throwing (you'd have to check, I can't remember).

• I have grown to be a bit skeptical of these blanket/flat/declarative xhr/fetch instrumentation configuration options.

  To be clear, I think declarative is good, in general. However, I'm skeptical that it is a good idea to apply configurations like these to all requests on the page indiscriminately. Scripts like intercom, or even the end-users' browser extensions, can easily add "unexpected" requests to the page. Even without the "first-party" requests, there are often times you want to collect more or less information conditionally (e.g. when A/B testing, when rolling out a new feature, deeply debugging a particular request, for certain types of users). It's kind of like logging, a global spout for filtering all kind of logs by log level is limited in its usefulness.

  Overtime, I think we have collected a few configurations that addresses this in a somewhat ad-hoc way and not very composable way, and I can end up similar to those. Whether it's ok to accept another feature and deal with the problem down the road, or take the opportunity to do something more systematic, is ultimately a judgement call (that isn't mine to make).

  However, consider that applyCustomAttributesOnSpan already exists, and can be used to do this plus any arbitrary conditional filtering you may want. So technically this isn't introducing any new functionality that aren't already possible today. To that end, since this is really just about improving ergonomics, I think it's fair to question whether it is worth adding without considering the bigger picture.

[chancancode]

The spec expects "normalized HTTP Header name (lowercase)" so this test is a bit misleading/wrong