-
Notifications
You must be signed in to change notification settings - Fork 8
ENG-56723: Add optional token as header, add specs #244
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #244 +/- ##
==========================================
+ Coverage 60.39% 61.17% +0.77%
==========================================
Files 59 59
Lines 2654 2684 +30
==========================================
+ Hits 1603 1642 +39
+ Misses 973 962 -11
- Partials 78 80 +2 ☔ View full report in Codecov by Sentry. |
I think we can allow for configuration of the token header name but have a reasonable default. |
| headers map[string]string | ||
| } | ||
|
|
||
| func WithHeaders(headers map[string]string) ServiceOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option pattern is a fairly popular pattern in go.
More such things can be read here: https://abhinavg.net/2022/12/06/designing-go-libraries/
This guy used to lead go team at uber.
| opts.headers = make(map[string]string) | ||
| } | ||
| for k, v := range headers { | ||
| opts.headers[k] = v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can probably use maps.Copy() ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL 👍
After working on the ext_cap -> goagent-src -> goagent changes I realized the previous approach wouldn't work because the exporterFactory is only inited once, after that the config is static & it's really just the
exporterFactory()that is invoked and needs to be aware of the new header.I've updated the approach to instead add a
WithHeadersoption we can pass down to customize the exporter that will be created from factory.This also removes the original question about hardcoding constant for the key, we'll be able to control headers from TPA, which may be nice from a maintenance perspective.
ORIGINAL
This optionally adds token header to otlpgrpc, otlphttp, or zipkin.
Q1: Do we have an expected key defined? This is currently marked as TODO- in the code.
Q2: Do we want key hardcoded constant for the header key or do we want the key also passed through TPA?
We already have a
Tokenfield in the config, if we want to customize key we will need to update config to allow a map of headers or add a TokenKey/TokenValue for ex.Please lmk direction we want to go, this impl is straightforward; however, allowing header key specification would be more robust - but from my current understanding maybe unnecessary.