Skip to content
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(otlp-exporter): support user-agent headers in web exporters #3811

Closed

Conversation

llc1123
Copy link
Contributor

@llc1123 llc1123 commented May 17, 2023

Which problem is this PR solving?

Support setting user-agent header in web exporters.

Fixes #3808

Update:

  • Will replace xhr with fetch in another PR.

Short description of the changes

  • Add user-agent header in web exporters in base packages.
  • Remove the use of sendBeacon which doesn't support custom headers.
  • Move the action of setting the user-agent header into base packages.
  • Warn the user if they try to config the user agent by config.
  • Correct the applying order of headers/metadata.
  • Normalize HTTP headers to lower case to prevent duplicate headers because they are case insensitive.
  • Fix a bug that env vars overwrite config from constructor parameters

Type of change

Please delete options that are not relevant.

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

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

  • Existing Tests

Checklist:

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

@llc1123 llc1123 force-pushed the fix/otlp-exporter-user-agent branch 2 times, most recently from 8111f47 to 3c01ac3 Compare May 17, 2023 09:02
@codecov
Copy link

codecov bot commented May 17, 2023

Codecov Report

Merging #3811 (bdab866) into main (2fb1b30) will decrease coverage by 0.04%.
The diff coverage is 93.33%.

❗ Current head bdab866 differs from pull request most recent head f3f0ffd. Consider uploading reports for the commit f3f0ffd to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3811      +/-   ##
==========================================
- Coverage   92.95%   92.92%   -0.04%     
==========================================
  Files         297      297              
  Lines        9060     9068       +8     
  Branches     1848     1852       +4     
==========================================
+ Hits         8422     8426       +4     
- Misses        638      642       +4     
Impacted Files Coverage Δ
...grpc-exporter-base/src/OTLPGRPCExporterNodeBase.ts 72.91% <75.00%> (-0.90%) ⬇️
...ter-base/src/platform/node/OTLPExporterNodeBase.ts 51.61% <80.00%> (+1.61%) ⬆️
...ges/exporter-logs-otlp-grpc/src/OTLPLogExporter.ts 93.10% <100.00%> (+0.79%) ⬆️
.../exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts 93.10% <100.00%> (+0.24%) ⬆️
...e-otlp-http/src/platform/node/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...-otlp-proto/src/platform/node/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts 93.75% <100.00%> (+0.20%) ⬆️
...-otlp-http/src/platform/node/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...orter-metrics-otlp-proto/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...perimental/packages/otlp-exporter-base/src/util.ts 93.33% <100.00%> (+0.22%) ⬆️
... and 1 more

... and 1 file with indirect coverage changes

@llc1123 llc1123 force-pushed the fix/otlp-exporter-user-agent branch 8 times, most recently from 0118b41 to f87d8d1 Compare May 18, 2023 04:32
@llc1123 llc1123 marked this pull request as ready for review May 18, 2023 04:40
@llc1123 llc1123 requested a review from a team May 18, 2023 04:40
@llc1123 llc1123 changed the title [otlp-exporter] support user-agent headers in web exporters feat(otlp-exporter): support user-agent headers in web exporters May 18, 2023
@llc1123 llc1123 force-pushed the fix/otlp-exporter-user-agent branch from f87d8d1 to 361c1ac Compare May 22, 2023 17:52
Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

I added my suggestion inline in case my previous comment wasn't clear. What do you think?

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

I'd like to see some duplication reduction and the beacon sending should not be removed in this PR.

I also think this PR is quite large and each changelog entry should have its own PR so that the changes can be reviewed independently.

* feat(opencensus-shim): add OpenCensus trace shim [#3809](https://github.com/open-telemetry/opentelemetry-js/pull/3809) @aabmass

### :bug: (Bug Fix)

* fix(sdk-node): use resource interface instead of concrete class [#3803](https://github.com/open-telemetry/opentelemetry-js/pull/3803) @blumamir
* fix(sdk-logs): remove includeTraceContext configuration and use LogRecord context when available [#3817](https://github.com/open-telemetry/opentelemetry-js/pull/3817) @hectorhdzg
* fix(otlp-exporters): correct applying order of http headers/gRPC metadata [#3811](https://github.com/open-telemetry/opentelemetry-js/pull/3811) @llc1123
Copy link
Member

Choose a reason for hiding this comment

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

This changelog doesn't really describe what changed. Also, should this change be in the same PR or a separate one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the applying order of headers/metadata is not fixed, the implementation of user-agent header in base packages will not work. That's why #3790 #3806 chose to add the header in separate packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had a discussion here.

Comment on lines +78 to +85
sendWithXhr(
body,
this.url,
this._headers,
this.timeoutMillis,
resolve,
reject
);
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change. Beacon sending is an important feature that can't be removed without significant reasoning and discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user-agent header is required, and sendBeacon() doesn't support HTTP headers at all. Also, the sendBeacon function is not used anywhere else. I think it should be safe removing it.

Copy link
Member

Choose a reason for hiding this comment

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

This is an environment specific requirement and there is precedent for breaking from spec for such reasons. Without beacon sending telemetry can be lost in many cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do know the advantages of sendBeacon. But the user-agent header is required. The sendBeacon function will never be reached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using fetch with keepalive=true also allows the request to outlive the page. If we can replace xhr with fetch.

Copy link
Contributor

Choose a reason for hiding this comment

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

fetch with keep-alive is not supported in all environments, so we can't rely on simply using it instead.
While (most) current versions of environments that support sendBeacon also support fetch keep alive, my understanding is that it is still considered to be expermenital.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Further discussion: #3845

Copy link
Member

Choose a reason for hiding this comment

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

regardless I think the refactor should be done separately from this PR as it is a very significant change

* @param onSuccess
* @param onError
*/
export function sendWithBeacon(
Copy link
Member

Choose a reason for hiding this comment

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

This cannot be removed without a serious discussion and significant user benefit

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @dyladan during page unloading sendBeacon and fetch (with keepalive) are the only options for getting data off the browser. And fetch with keep-alive is not always supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked the compatibility of these options. ~5% of the browsers support sendBeacon but not keepalive.
Maybe we should implement with fetch and use sendBeacon as a fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Further discussion: #3845

@llc1123
Copy link
Contributor Author

llc1123 commented May 31, 2023

I'd like to see some duplication reduction and the beacon sending should not be removed in this PR.

I also think this PR is quite large and each changelog entry should have its own PR so that the changes can be reviewed independently.

There are 3 major changes, which are dependent each other.

  • The current implementation of merging headers/metadata is totally wrong, headers are applied in incorrect order.
  • To make the user-agent header work properly, the above one should be fixed.
  • sendBeacon will not be used anymore (it doesn't support HTTP headers) and should be removed.

@llc1123
Copy link
Contributor Author

llc1123 commented May 31, 2023

If this PR is too large, I would like to split it into following PRs:

  • Fix the applying order of config and env vars.
    • Fix other env vars related issues.
  • Replace xhr and sendBeacon with fetch (keepalive=true).
  • Normalize the http header to lower cases.
  • Add the user-agent header to base packages.

Copy link
Contributor

@MSNev MSNev left a comment

Choose a reason for hiding this comment

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

Do not remove sendBeacon support

@llc1123
Copy link
Contributor Author

llc1123 commented May 31, 2023

Further discussion about the use of sendBeacon: #3845

@dyladan
Copy link
Member

dyladan commented May 31, 2023

If this PR is too large, I would like to split it into following PRs:

  • Fix the applying order of config and env vars.

    • Fix other env vars related issues.
  • Replace xhr and sendBeacon with fetch (keepalive=true).

  • Normalize the http header to lower cases.

  • Add the user-agent header to base packages.

I would support that

@dyladan
Copy link
Member

dyladan commented Jul 26, 2023

Any update on this?

@llc1123
Copy link
Contributor Author

llc1123 commented Jul 26, 2023

Any update on this?

It's blocked by #3845 I think.

@pichlermarc
Copy link
Member

Closing this for now - the changes proposed by PR were very controversial around removing sendBeacon.
I'm about to open a PR which proposes more modular exporters, I'll start with node.js will take on browser exporters in a follow-up, which will include user-agent headers when XHR is used for sending.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[otlp-exporter] web exporters should set User-Agent header upon requests.
5 participants