feat(otlp-trace-exporters): Add User-Agent header to OTLP trace exporters

[JamieDanielson]

Which problem is this PR solving?

Updates #3291

Short description of the changes

• add user agent to otlp grpc trace exporter
• add user agent to otlp http/json trace exporter
• add user agent to otlp http/proto trace exporter
• add unit tests for the above changes

I tried adding this to the browser exporter but get an error for setting an Unsafe Header.
This should no longer be forbidden but I wasn't sure what I was missing.
Edit: I believe this was erroring because it would overwrite the existing User Agent. Either way, this PR does not make changes to the browser implementation and instead focuses on just Node exporters.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• Unit tests

Checklist:

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

[codecov]

Codecov Report

Merging #3790 (3b8210b) into main (422a36a) will decrease coverage by 0.69%.
The diff coverage is 100.00%.

❗ Current head 3b8210b differs from pull request most recent head 5d6fcc4. Consider uploading reports for the commit 5d6fcc4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3790      +/-   ##
==========================================
- Coverage   93.22%   92.54%   -0.69%     
==========================================
  Files         293      282      -11     
  Lines        8883     8326     -557     
  Branches     1825     1721     -104     
==========================================
- Hits         8281     7705     -576     
- Misses        602      621      +19     

Impacted Files	Coverage Δ
.../exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts	92.85% <100.00%> (+0.54%)	⬆️
...e-otlp-http/src/platform/node/OTLPTraceExporter.ts	100.00% <100.00%> (ø)
...-otlp-proto/src/platform/node/OTLPTraceExporter.ts	100.00% <100.00%> (ø)

... and 13 files with indirect coverage changes

[llc1123]

I believe it is better making the implementations in the base packages (otlp-exporter-base, otlp-grpc-exporter-base and otlp-proto-exporter-base), which also work as the base packages of metrics/logs exporters.

[llc1123]

This should no longer be forbidden but I wasn't sure what I was missing.

That depends on browser implementation. Chrome Browser currently doesn't support that. I think we can just ignore the warnings.

[JamieDanielson]

I believe it is better making the implementations in the base packages (otlp-exporter-base, otlp-grpc-exporter-base and otlp-proto-exporter-base), which also work as the base packages of metrics/logs exporters.

I started there, but because of the way the headers are built currently, I need them added in the individual exporters. The headers for each exporter are intended to override the generic variables. For example, the OTEL_EXPORTER_OTLP_TRACES_HEADERS are used over OTEL_EXPORTER_OTLP_HEADERS for traces, so even if I set it in base it will get clobbered by the trace-specific setup. If I set them in the base packages, I'll have to update each of these constructors anyway. I'm open to trying that if others agree that is the best way to do this.

[JamieDanielson]

This should no longer be forbidden but I wasn't sure what I was missing.

That depends on browser implementation. Chrome Browser currently doesn't support that. I think we can just ignore the warnings.

I was getting errors when running tests. It's possible the issue is that I tried overwriting user agent instead of appending 🤔 I'm definitely less familiar with the browser implementations.

        Error: Uncaught AssertionError: 'Refused to set unsafe header "User-Agent"' === 'Request Timeout' (webpack-internal:///../../../node_modules/assert/assert.js:199)

[llc1123]

I believe it is better making the implementations in the base packages (otlp-exporter-base, otlp-grpc-exporter-base and otlp-proto-exporter-base), which also work as the base packages of metrics/logs exporters.

I started there, but because of the way the headers are built currently, I need them added in the individual exporters. The headers for each exporter are intended to override the generic variables. For example, the OTEL_EXPORTER_OTLP_TRACES_HEADERS are used over OTEL_EXPORTER_OTLP_HEADERS for traces, so even if I set it in base it will get clobbered by the trace-specific setup. If I set them in the base packages, I'll have to update each of these constructors anyway. I'm open to trying that if others agree that is the best way to do this.

Let's see what we can do after #3748 being fixed which will change the way headers are merged.

[pichlermarc]

Thank you for adding this 🙂
I've tried this before but ran into the exact same problem with it not working in the base exporter. 😞
(I'm actually working to refactor some the exporters/exporter bases to make them easier to work with - hopefully this will help in simplifying how we build the headers, but I think this PR is a great first step. 🙂)

[martinkuba]

Looks good to me with the caveat that backends consuming browser data will not be able to rely on the User-Agent header to determine the browser brand/version. This just highlights the importance of using the browser resource attributes instead.