Stats/Stackdriver: Add user agent as client header #320
Conversation
|
I had offline discussion with @JustinBeckwith, decided to add options at |
| (err: Error) => { | ||
| this.logger.debug( | ||
| 'sent time series', request.resource.timeSeries); | ||
| err ? reject(err) : resolve(); |
There was a problem hiding this comment.
Optional nit: since you are checking for the presence of err, should you have the parameter be err?: Error?
(Although the package doesn't have strict null checks enabled, I think it's still helpful to think in terms of them and then if eventually we want to turn them on they will be easier to enable).
| err ? reject(err) : resolve(); | ||
| }); | ||
| monitoring.projects.metricDescriptors.create( | ||
| request, {headers: OC_HEADER, userAgentDirectives: [OC_USER_AGENT]}, |
There was a problem hiding this comment.
Will you need similar logic for other types of Google API calls in the exporter, e.g. writing traces?
If so, what would you think about exporting the larger {headers: OC_HEADER, userAgentDirectives: [OC_USER_AGENT]} constant as something like OC_HEADER_OPTIONS or similar and making it be in a shared utility between trace and metrics?
There was a problem hiding this comment.
As per my understanding, User Agent requirement is only Stackdriver stats exporter specific. For Stackdriver Trace exporter, we add agent information as a part of the label (example: g.co/agent -> opencensus-node [{version}]). This is implemented in all the libraries except node. Issue #39 -> to address that.
There was a problem hiding this comment.
Ah, makes sense, then keeping these constants in this file only seems fine.
Codecov Report
@@ Coverage Diff @@
## master #320 +/- ##
=========================================
- Coverage 94.93% 94.9% -0.04%
=========================================
Files 118 118
Lines 8101 8105 +4
Branches 723 723
=========================================
+ Hits 7691 7692 +1
- Misses 410 413 +3
Continue to review full report at Codecov.
|
Fixes #194