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

Zipkin Exporter does not include the headers from client settings in request #30679

Closed
EOjeah opened this issue Jan 19, 2024 · 7 comments
Closed
Labels
exporter/zipkin question Further information is requested

Comments

@EOjeah
Copy link
Contributor

EOjeah commented Jan 19, 2024

Component(s)

exporter/zipkin

What happened?

Description

According to HTTP Configuration Settings Exporters can leverage client configuration.

Zipkin Exporter specifies you can leverage these advanced settings from README.md, however it does not properly update the headers when you put that in the configuration.

Steps to Reproduce

  • Set headers in the zipkin exporter configuration
exporters:
  zipkin:
    endpoint: "https://<zipkin-url>/api/v2/spans"
    format: proto
    headers:
      "any-key": "value"

Actual Result

Print your req struct from zipkin.go with fmt.Println(req)

  • Result will print a Go struct and i have included the mapping from struct so result looks like:
&{
  Method      POST
  URL         https://<zipkin-url>/api/v2/spans
  Proto       HTTP/1.1
  ProtoMajor  1 
  ProtoMinor  1 
  Header      map[Content-Type:[application/x-protobuf]]
...
}

Expected Result

&{
  Method      POST
  URL         https://<zipkin-url>/api/v2/spans
  Proto       HTTP/1.1
  ProtoMajor  1 
  ProtoMinor  1 
- Header      map[Content-Type:[application/x-protobuf]]
+ Header      map[Content-Type:[application/x-protobuf] Test:[value]]
...
}

Collector version

v0.92.0

Environment information

Environment

OS: Ubuntu 22.04.3 LTS
Compiler: go1.21.5 linux/amd64

OpenTelemetry Collector configuration

receivers:
  zipkin:

exporters:
  zipkin:
    endpoint: "https://<zipkin>/api/v2/spans"
    format: proto
    headers:
      Test: "value"

service:
  pipelines:
    traces:
      receivers: [zipkin]
      exporters: [zipkin]

Log output

No response

Additional context

No response

@EOjeah EOjeah added bug Something isn't working needs triage New item requiring triage labels Jan 19, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@crobert-1
Copy link
Member

Hello @EOjeah, the headers defined in the config are actually set in the call stack of zipkin.go's call to ze.client.Do. They're being set here, so you won't see them in the req object in the pushTraces method.

Here's the call stack showing how we get to the method that's setting the headers set in the config:

confighttp.(*headerRoundTripper).RoundTrip (confighttp.go:239) go.opentelemetry.io/collector/config/confighttp
otelhttp.(*Transport).RoundTrip (transport.go:116) go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp
http.send (client.go:260) net/http
http.(*Client).send (client.go:181) net/http
http.(*Client).do (client.go:724) net/http
http.(*Client).Do (client.go:590) net/http
zipkinexporter.(*zipkinExporter).pushTraces (zipkin.go:83) github.com/open-telemetry/opentelemetry-collector-contrib/exporter/zipkinexporter
<autogenerated>:2
exporterhelper.(*tracesRequest).Export (traces.go:58) go.opentelemetry.io/collector/exporter/exporterhelper
exporterhelper.(*timeoutSender).send (timeout_sender.go:43) go.opentelemetry.io/collector/exporter/exporterhelper
exporterhelper.(*retrySender).send (retry_sender.go:89) go.opentelemetry.io/collector/exporter/exporterhelper
exporterhelper.(*tracesExporterWithObservability).send (traces.go:171) go.opentelemetry.io/collector/exporter/exporterhelper
exporterhelper.newQueueSender.func1 (queue_sender.go:115) go.opentelemetry.io/collector/exporter/exporterhelper
internal.(*boundedMemoryQueue).Consume (bounded_memory_queue.go:57) go.opentelemetry.io/collector/exporter/exporterhelper/internal
internal.(*boundedMemoryQueue).Consume (bounded_memory_queue.go:50) go.opentelemetry.io/collector/exporter/exporterhelper/internal
internal.(*QueueConsumers).Start.func1 (consumers.go:43) go.opentelemetry.io/collector/exporter/exporterhelper/internal
runtime.goexit (asm_amd64.s:1650) runtime
 - Async Stack Trace
internal.(*QueueConsumers).Start (consumers.go:39) go.opentelemetry.io/collector/exporter/exporterhelper/internal

Is there a specific reason besides what you've shared that you made it seem like headers weren't being sent properly with the exporter requests?

@crobert-1 crobert-1 added question Further information is requested and removed bug Something isn't working needs triage New item requiring triage labels Jan 19, 2024
@EOjeah
Copy link
Contributor Author

EOjeah commented Jan 19, 2024

@crobert-1 thanks for looking through.

Actually I came across it when configuring the collector to send to zipkin server behind nginx.

It seems the Host header is extracted from the URL but my misconfiguration from nginx meant it expected a different Host header value from the dns name. When I tried setting this, I still got a 502 response from the server because Host did not match what was expected

I do think the headers are set but not sure it includes the headers from the common Client Settings. Potentially an easy fix I've seen some other exporters use. #30685

@crobert-1
Copy link
Member

Thanks for including more information!

The method I shared before is adding the headers specified in the config. interceptor in that context was set during the startup of the exporter here. From the setting of headers: hcs.Headers, we can see that the interceptor.headers loop is referencing the configured headers from HTTPClientSettings. I confirmed this while debugging, the PR you've posted is just duplicating the existing logic.

Does that make sense, did I miss something?

@EOjeah
Copy link
Contributor Author

EOjeah commented Jan 19, 2024

ah that makes sense thanks for the info! I think I may have missed something on my end and will check again next week

@EOjeah EOjeah closed this as not planned Won't fix, can't repro, duplicate, stale Jan 19, 2024
@andrzej-stencel
Copy link
Member

Does this work for you @EOjeah? There was an issue discussed yesterday in the Collector SIG meeting that makes specifying the Host header ineffective. Here's the issue: open-telemetry/opentelemetry-collector#9395 and the pull request that's hopefully going to fix this soon: open-telemetry/opentelemetry-collector#9411.

@EOjeah
Copy link
Contributor Author

EOjeah commented Feb 5, 2024

@astencel-sumo yeah this will probably solve the issue I had as well. The Host header was expecting something different from the header extracted from the endpoint url (Go's net/http library). I made changes to the proxy I used so the Host header required matched what was in the endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/zipkin question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants