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

Can't Set Authorization Headers for Zipkin Exporter #1200

Closed
astorm opened this issue Jun 16, 2020 · 8 comments · Fixed by #1202
Closed

Can't Set Authorization Headers for Zipkin Exporter #1200

astorm opened this issue Jun 16, 2020 · 8 comments · Fixed by #1202
Assignees

Comments

@astorm
Copy link
Contributor

astorm commented Jun 16, 2020

Is your feature request related to a problem? Please describe.

I would like to send trace data to a Zipkin compatible endpoint. This Zipkin endpoint wants me to set a few additional HTTP headers (an auth token/API Key being the most important). The current Zipkin exporter does not appear to let me do this. This is based on my trying to use the exporter in a real world situation -- it is not hypothetical.

Describe the solution you'd like
I would like the ZipkinExporter's constructor to accept an optional list of headers as part of the Zipkin configuration. I would also like a public method on the ZipkinExporter that would allow these headers (or perhaps all the request options?) to be updated at anytime. The use case for this public method would be an authorization token that changes with a long running application. I am not strongly tied to these implementation details -- my main goal is being able to set additional HTTP headers for the Zipkin exporter.

Describe alternatives you've considered
I have considered using the existing API like this

const zipkinExporter = new ZipkinExporter({
  url: 'http://zipkin.example.com:9411/api/v2/spans',
  serviceName: 'service-hello'
});

// set additional headers if needed -- for example, New Relic's
// zipkin trace endpoint: https://docs.newrelic.com/docs/understand-dependencies/distributed-tracing/trace-api/report-zipkin-format-traces-trace-api
zipkinExporter._reqOpts.headers['Api-Key'] = 'key-value-here'
zipkinExporter._reqOpts.headers['Data-Format'] = 'zipkin'    
zipkinExporter._reqOpts.headers['Data-Format-Version'] = '2'   

but this will make some people on my team sad (some days I am one of those people), and is also fragile and likely to break if/when the underlying implementation of the exporter changes. It will also not work for folks directly using TypeScript with a certain level of strictness.

Additional context
If this is possible to do in a non-fragile way, please let me know. Also, if this is something you would like in the project but can't prioritize, I think we could make a PR happen.

@dyladan
Copy link
Member

dyladan commented Jun 16, 2020

Which "Zipkin compatible endpoint" are you using? Is it a well-known service that adding a config for that particular service's API key would be better?

@dyladan
Copy link
Member

dyladan commented Jun 16, 2020

Just having a config that allows a key-value map that applies as an HTTP header seems like it is likely only useful for this particular setup. Maybe we should have some config for a request-transformer hook that is called with the request object before it is sent? This would allow other things like dynamic headers

@obecny
Copy link
Member

obecny commented Jun 16, 2020

I think it makes perfect sense and it shouldn't be complicated. Moreover I think it could be aligned across all exporters WDYT @mayurkale22 . If that is a case I would be happy to take it and implement for our exporters

@obecny
Copy link
Member

obecny commented Jun 16, 2020

@dyladan I would add 2 methods addHeaders and removeHeaders WDYT ?

@dyladan
Copy link
Member

dyladan commented Jun 16, 2020

I would just make it a config option httpRequestHeaders. No need to add/remove them after the exporter is created

@astorm
Copy link
Contributor Author

astorm commented Jun 16, 2020

Thanks for taking a look @dyladan and @obecny

Which "Zipkin compatible endpoint" are you using? Is it a well-known service that adding a config for that particular service's API key would be better?

The specific real world problem I ran into was a New Relic endpoint that accepts Zipkin formatted spans but wants a few additional headers (including an Api-Key) to be set: https://docs.newrelic.com/docs/understand-dependencies/distributed-tracing/trace-api/report-zipkin-format-traces-trace-api

I think adding a config for a particular service's API key would be interesting, and I'm sure the marketing folks at New Relic would love it, but I don't think it's the right approach here. A protocol can change -- today this endpoint uses Api-Key -- but in 6 months it might us an oAuth token, or some other auth mechanism. Also, who can say what sort of things folks are going to do in the future -- adding configuration for specific providers is going to be a whack-a-mole affair. I have enough of those in my life already :)

The exporter currently allows us to configure the URL that Zipkin spans are posted to, and also allows indirect setting of request options (via URL parsing). By expanding this slightly to include headers we're still within the spirit of the current exporter.

Maybe we should have some config for a request-transformer hook that is called with the request object before it is sent? This would allow other things like dynamic headers

A request transformer sounds neat -- but also feels like more complexity than is warranted right now. This may just be an old web developer talking but being able to set headers on an outgoing request feels as natural as setting the URL. I'd say if the project sees more than three requests around tweaking request settings then that would be the time to look for a more general solution.

I would add 2 methods addHeaders and removeHeaders

My original FR did ask for this -- but given my simplification argument above these methods aren't strictly needed for the current New Relic endpoint I'm targeting. Adding an argument to the config seems like the quickest and cleanest path forward right now, and can be revisited if/when there's an endpoint that requires a running application to change its config. Also, it's worth noting that if an application did require this then it could add a new exporter with the new configuration.

What do you'all think about the above?

@dyladan
Copy link
Member

dyladan commented Jun 16, 2020

I think it sounds reasonable and @obecny has already opened a PR with the config option #1202

@astorm
Copy link
Contributor Author

astorm commented Jun 18, 2020

Thanks all -- you are a mighty machine capable of swift action!

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants