Stackdriver exporter: Allow overriding client options via config #1010
Conversation
| // to the underlying Cloud Monitoring API client. | ||
| // Must be set programmatically (no support via declarative config). | ||
| // Optional. | ||
| ClientOptions []option.ClientOption |
There was a problem hiding this comment.
Optional: I wonder if it would be cleaner to put this into a separate struct that is passed into the NewFactory() function explicitly instead of polluting the Config struct?
There was a problem hiding this comment.
The problem with making it part of NewFactory is that it will be "static", i.e. will apply to all instances of exporter.
One could imagine configuring exporters with different client options.
Codecov Report
@@ Coverage Diff @@
## master #1010 +/- ##
===========================================
+ Coverage 74.88% 88.77% +13.89%
===========================================
Files 21 247 +226
Lines 1071 13274 +12203
===========================================
+ Hits 802 11784 +10982
- Misses 213 1135 +922
- Partials 56 355 +299
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| // to the underlying Google Cloud API client. | ||
| // Must be set programmatically (no support via declarative config). | ||
| // Optional. | ||
| GetClientOptions func() []option.ClientOption |
There was a problem hiding this comment.
Switched to callback function to make it clear that this field can only be set programmatically.
| @@ -65,16 +65,29 @@ func (me *metricsExporter) Shutdown(context.Context) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func generateClientOptions(cfg *Config, dialOpts ...grpc.DialOption) ([]option.ClientOption, error) { | |||
| func generateClientOptions(cfg *Config, userAgent string) ([]option.ClientOption, error) { | |||
There was a problem hiding this comment.
Consolidated all logic initializing client options in one place.
| @@ -126,21 +137,12 @@ func newStackdriverMetricsExporter(cfg *Config, version string) (component.Metri | |||
| } | |||
|
|
|||
| userAgent := strings.ReplaceAll(cfg.UserAgent, "{{version}}", version) | |||
There was a problem hiding this comment.
Bring this line into generateClientOptions as well?
There was a problem hiding this comment.
Done, had to change the trace exporter constructor though (which is probably for the better).
Description: Currently we have to use a forked Stackdriver exporter for custom authorization (#688).
Adding ability to override client options will only require to make a "wrapper" factory without having a complete fork of the code.
Link to tracking Issue: #688
Testing: Added a basic unit test overriding client options
Documentation: Given that a new config parameter is meant to be overridden programmatically by developers, there is no need for user documentation.