-
Notifications
You must be signed in to change notification settings - Fork 573
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
[config] add NewSDK func #4414
[config] add NewSDK func #4414
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4414 +/- ##
=======================================
- Coverage 82.3% 80.8% -1.5%
=======================================
Files 144 148 +4
Lines 10019 10240 +221
=======================================
+ Hits 8248 8283 +35
- Misses 1633 1819 +186
Partials 138 138
|
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.
Overall looks good 👍
// WithOpenTelemetryConfiguration sets the OpenTelemetryConfiguration used | ||
// to produce the SDK. | ||
func WithOpenTelemetryConfiguration(cfg OpenTelemetryConfiguration) ConfigurationOption { | ||
return configurationOptionFunc(func(c configOptions) configOptions { | ||
c.opentelemetryConfig = cfg | ||
return c | ||
}) | ||
} |
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.
Shouldn't cfg OpenTelemetryConfiguration
be a required parameter?
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.
It could definitely be, i suppose not setting it would be useful for someone who wants a noop SDK... not sure if that's a realistic use-case, but i figured it would be easier to be consistent w/ all options.
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 suppose not setting it would be useful for someone who wants a noop SDK...
Interesting use case. For me you can leave it as it is. You can resolve the comment or ask/wait for other opinions.
Please add a changelog entry 😉 |
For now it's returning a noop tracer/meter provider if no configuration is found. Otherwise it returns an unconfigured SDK meter/tracer provider. This PR is mostly to start putting the pieces together for the interface. Signed-off-by: Alex Boten <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
b1d368a
to
8178684
Compare
The failing tests appear unrelated... |
True. I created #4416 |
Anything else needed for this PR before merging? |
For now it's returning a noop tracer/meter provider if no configuration is found. Otherwise it returns an unconfigured SDK meter/tracer provider.
This PR is mostly to start putting the pieces together for the interface. For reviewers: I decided to go w/ passing in options rather than specific parameters into NewSDK.
Related to #4371