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: collector exporter custom headers and metadata #1204

Merged
merged 7 commits into from
Jun 18, 2020

Conversation

mwear
Copy link
Member

@mwear mwear commented Jun 17, 2020

Which problem is this PR solving?

Short description of the changes

  • This PR adds support custom headers or grpc metadata depending on the transport. Both are specified as configuration options when initializing the collector exporter.

@mwear mwear added enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 17, 2020
@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #1204 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1204   +/-   ##
=======================================
  Coverage   92.32%   92.32%           
=======================================
  Files         122      122           
  Lines        3533     3533           
  Branches      714      714           
=======================================
  Hits         3262     3262           
  Misses        271      271           

super(config);
this._headers = config.headers || this.DEFAULT_HEADERS;
this._useXHR =
!!config.headers || typeof navigator.sendBeacon !== 'function';
Copy link
Member

Choose a reason for hiding this comment

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

should this be !!this._headers || ..., or do we explicitly not care about sending only DEFAULT_HEADERS here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there are custom headers to be sent, then spans must be sent using an XMLHTTPRequest, the beacon doesn't support arbitrary headers. If there are not custom headers, and the browser has beacon support, spans will be sent using it.

There is one default header, it's collectorTypes.OT_REQUEST_HEADER which is a signal to the XMLHTTPRequest plugin not to trace the request. This would come into play if there are not any custom headers, and the browser does not have beacon support. A better long term solution for this would be #1040. It would clean this up and we could remove default headers altogether.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense thanks!

@mwear mwear requested a review from legendecas as a code owner June 17, 2020 18:47
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, thx for changes

@mayurkale22 mayurkale22 merged commit 28ad2ce into open-telemetry:master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adds possibility to set headers to Collector Exporter
5 participants