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

bug(exporter-collector): headers are appended to existing one #2335

Closed
niko-achilles opened this issue Jul 7, 2021 · 9 comments · Fixed by #2357
Closed

bug(exporter-collector): headers are appended to existing one #2335

niko-achilles opened this issue Jul 7, 2021 · 9 comments · Fixed by #2357
Assignees
Labels
bug Something isn't working

Comments

@niko-achilles
Copy link
Contributor

niko-achilles commented Jul 7, 2021

Please answer these questions before submitting a bug report.

What version of OpenTelemetry are you using?

"@opentelemetry/context-zone": "^0.23.0",
"@opentelemetry/exporter-collector": "^0.23.0",
"@opentelemetry/instrumentation": "^0.23.0",
"@opentelemetry/auto-instrumentations-web": "^0.23.0",
"@opentelemetry/propagator-b3": "^0.23.0",
"@opentelemetry/tracing": "^0.23.0",
"@opentelemetry/web": "^0.23.0",
"@opentelemetry/resources": "0.23.0",
"@opentelemetry/semantic-conventions": "0.23.0",

Collector docker image: otel/opentelemetry-collector-contrib:0.29.0 and/or otel/opentelemetry-collector:0.29.0

What version of Node are you using?

14 --lts : latest

Please provide the code you used to setup the OpenTelemetry SDK

https://github.com/niko-achilles/otlp-logzio-fullstack/blob/main/frontend/lib/useOpentelemetry.js#L25

What did you do?

If i provide explicitly , the content type of application/json as follows:

provider.addSpanProcessor(
    new SimpleSpanProcessor(
      new CollectorTraceExporter({
        url: "http://localhost:55681/v1/traces",
        headers: {"Content-Type": "application/json"},
      })
    )
  );

Then the zone.js script makes a request of type xhr and applies in content-type application/json twice ,
Content-type: application/json application/json

If i do not provide explicitly the Content-Type in the headers object , then
zone.js script makes a request of type xhr and applies in content-type : text/plain;charset=utf8

What did you expect to see?

content-type: application/json in the request of zone.js .

What did you see instead?

zone.js script makes a request of type xhr and applies in content-type : text/plain;charset=utf8
or
zone.js script makes a request of type xhr and applies in content-type application/json twice ,
Content-type: application/json application/json.

Additional context

If i write implicit , having the headers object as follows:

provider.addSpanProcessor(
    new SimpleSpanProcessor(
      new CollectorTraceExporter({
        url: "http://localhost:55681/v1/traces",
        headers: {},
      })
    )
  );

Then the traces are collected, because zone.js applies as content-type: application/json

Add any other context about the problem here.

@niko-achilles niko-achilles added the bug Something isn't working label Jul 7, 2021
@vmarchaud
Copy link
Member

This is duplicate with #2321 so i'll close this one, the problem comes from the collector exporter / collector itself and not zonejs

@niko-achilles
Copy link
Contributor Author

niko-achilles commented Jul 11, 2021

hi @vmarchaud , this issue was raised because of the usage of CollectorTraceExporter api .
The situation is thatbform developer point of view the explicit setup CollectorTraceExporter as follows:

provider.addSpanProcessor(
    new SimpleSpanProcessor(
      new CollectorTraceExporter({
        url: "http://localhost:55681/v1/traces",
        headers: {"Content-Type": "application/json"},
      })
    )
  );

results in a headers object where Content-Type is set twice. "Content-Type": "application/json application/json"

If i implicit define it as follows:

provider.addSpanProcessor(
    new SimpleSpanProcessor(
      new CollectorTraceExporter({
        url: "http://localhost:55681/v1/traces",
        headers: {},
      })
    )
  );

results are that the headers object hat a Content-Type of: "Content-Type": "application/json"

If i implicit omit the headers object it as follows:

provider.addSpanProcessor(
    new SimpleSpanProcessor(
      new CollectorTraceExporter({
        url: "http://localhost:55681/v1/traces",
      })
    )
  );

results are that the headers object hat a Content-Type of: "Content-Type": "plain/text;charset=utf-8"

Maybe the design of the CollectorTraceExporter api needs an enhancement for being more explicit than implicit ?
i mean this code :

provider.addSpanProcessor(
    new SimpleSpanProcessor(
      new CollectorTraceExporter({
        url: "http://localhost:55681/v1/traces",
        headers: {"Content-Type": "application/json"},
      })
    )
  );

should result in a headers object where Content-Type is application/json . "Content-Type": "application/json"

@vmarchaud
Copy link
Member

I believe i understood the issue which is duplicate of #2321 (fixes is here #2336), isn't that the case ?

@niko-achilles
Copy link
Contributor Author

@vmarchaud i think actually not .
This issue may rather be an improvement than a bug because:
if headers config is present , xhr is used
else if not headers config is present , beacon is used

Additionally if headers config is present and the application developer provides explicit the Content-Type: "application/json"
then xhr is used and the Content Type is applied twice: "Content-Type": "application/json application/json"

@vmarchaud
Copy link
Member

if headers config is present , xhr is used
else if not headers config is present , beacon is used

This is because headers can't be used as-is with beacon (see this), thats one of the things that #2336 should fix.

Additionally if headers config is present and the application developer provides explicit the Content-> Type: "application/json"
then xhr is used and the Content Type is applied twice: "Content-Type": "application/json application/json"

Could you provide a full example of this ? I can't reproduce locally

@niko-achilles
Copy link
Contributor Author

sure, setup: https://github.com/niko-achilles/otlp-logzio-fullstack/blob/dev/frontend/lib/useOpentelemetry.js#L29

then in network tab of chrome dev tools i see this:

Screenshot from 2021-07-12 12-45-52

Notice:
Request Headers -> Content-Type: application/json application/json
applied twice.

@niko-achilles
Copy link
Contributor Author

hi @vmarchaud managed to reproduce locally given the infos above ? I am asking with the motivation that maybe i have done something wrong on the setup on my side ? i get Request Headers -> Content-Type: application/json application/json

@vmarchaud
Copy link
Member

I did not, i'll reopen this in the mean time but if you have the time to make PR that would be welcome :)

@vmarchaud vmarchaud reopened this Jul 13, 2021
@vmarchaud vmarchaud changed the title zone.js content-type using collector 0.29.0 bug(exporter-collector): headers are appended to existing one Jul 13, 2021
@vmarchaud vmarchaud added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Jul 13, 2021
@niko-achilles
Copy link
Contributor Author

@vmarchaud i checked the code in opentelemetry-js/utils : https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts#L59
And because of the XMLHttpRequestStandard as documented here: https://xhr.spec.whatwg.org/#the-setrequestheader()-method

when setting the same header twice , the value(a.k.a body) of the header is appended .

But before making a PR i would like to ask , how should an application developer use the api of CollectorExporter ?
I mean is the CollectorExporter config object only for usage of custom headers ? -> this is how it is mentioned in some opentelemetry examples .

Or should the api of CollectorExporter correct the header values, e.g. in case the developer applied a Content-Type of application/json then the api of CollectorExporter should not set the header value twice in this method https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts#L59 ?

niko-achilles added a commit to niko-achilles/opentelemetry-js that referenced this issue Jul 15, 2021
@vmarchaud vmarchaud removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Jul 15, 2021
niko-achilles added a commit to niko-achilles/opentelemetry-js that referenced this issue Jul 16, 2021
dyladan added a commit that referenced this issue Jul 21, 2021
* fix: headers are appended to existing one (#2335)

* test(browser-util): added 2 more test-cases refactored desc, it wording

* test: split tests review

* fix: review headers are appended to existing one (#2335)

* test: review DRY

* fix: review lint

Co-authored-by: Valentin Marchaud <[email protected]>
Co-authored-by: Daniel Dyla <[email protected]>
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 a pull request may close this issue.

2 participants