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

fix: headers are appended to existing one (open-telemetry#2335) #2357

Merged
merged 9 commits into from
Jul 21, 2021

Conversation

niko-achilles
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • added file util.test.ts under packages/opentelemetry-exporter-collector/test/browser/util.test.ts
    which exercises the sendWithXhr method of packages/opentelemetry-exporter-collector/src/platform/browser/util.ts
  • File packages/opentelemetry-exporter-collector/src/platform/browser/util.ts is changed to verify if the developer who uses the CollectorTraceExporter api has set explicit Content-Type and Accept Headers . If that is not the case , only then the sendWithXhrt method applies the Content-Type and Accept Headers with the value application/json.

@codecov
Copy link

codecov bot commented Jul 15, 2021

Codecov Report

Merging #2357 (e7f00df) into main (c55142f) will increase coverage by 0.30%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2357      +/-   ##
==========================================
+ Coverage   92.47%   92.77%   +0.30%     
==========================================
  Files         122      145      +23     
  Lines        4132     5221    +1089     
  Branches      859     1070     +211     
==========================================
+ Hits         3821     4844    +1023     
- Misses        311      377      +66     
Impacted Files Coverage Δ
...kages/opentelemetry-api-metrics/src/api/metrics.ts 90.90% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...ntelemetry-web/src/enums/PerformanceTimingNames.ts 100.00% <0.00%> (ø)
packages/opentelemetry-web/src/utils.ts 94.90% <0.00%> (ø)
...ry-context-zone-peer-dep/src/ZoneContextManager.ts 85.18% <0.00%> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts 95.71% <0.00%> (ø)
...opentelemetry-api-metrics/src/NoopMeterProvider.ts 100.00% <0.00%> (ø)
packages/opentelemetry-web/src/types.ts 100.00% <0.00%> (ø)
...es/opentelemetry-context-zone-peer-dep/src/util.ts 100.00% <0.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 96.91% <0.00%> (ø)
... and 13 more

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.

just one last thing, other than that lgtm

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm

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.

nice and neat :), lgtm thx for changes

@dyladan dyladan added the bug Something isn't working label Jul 21, 2021
@dyladan
Copy link
Member

dyladan commented Jul 21, 2021

Once the lint is fixed this is mergeable

@dyladan
Copy link
Member

dyladan commented Jul 21, 2021

think there might be a github UI bug...

image

image

@dyladan dyladan self-requested a review July 21, 2021 15:56
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.

Approving again to try to fix github

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(exporter-collector): headers are appended to existing one
5 participants