-
Notifications
You must be signed in to change notification settings - Fork 772
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
[sdk] Make ResourceBuilder.AddDetector factory pattern public #4141
[sdk] Make ResourceBuilder.AddDetector factory pattern public #4141
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4141 +/- ##
==========================================
+ Coverage 85.54% 85.58% +0.04%
==========================================
Files 293 293
Lines 11371 11371
==========================================
+ Hits 9727 9732 +5
+ Misses 1644 1639 -5
|
internal ResourceBuilder AddDetector(Func<IServiceProvider?, IResourceDetector> resourceDetectorFactory) | ||
public ResourceBuilder AddDetector(Func<IServiceProvider?, IResourceDetector> resourceDetectorFactory) |
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.
Spoke with @CodeBlanch offline a bit about this... This method seems kinda wonky in that it differs from our provider builders (tracer, meter, etc). I floated the idea of something like
public static ResourceBuilder AddDetector<T>(this ResourceBuilder resourceBuilder)
where T : IResourceDetector
{
return resourceBuilder.AddDetector(sp => sp.GetRequiredService<T>());
}
Though this is wonky for other reasons. It differs from methods like AddProcessor/Instrumentation/Sampler/etc on our provider builders in that this will not automatically add a detector of type T to the service collection.
Also, @CodeBlanch raised a point that ResourceBuilder is hard because it has a public constructor, so something like the following would never work
new ResourceBuilder().AddDetector<MyDetector>()
All that said, my hope is we can do something better here that feels more consistent with the provider builders. My vote is to hold off on this change until after 1.4. A workaround has been provided, albeit not great, but gives us a chance to consider alternatives.
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.
Sounds good to me I'll close this and we can work on it for a future release.
Fixes #4139
Relates to #4103
Changes
History
This API was originally put in to enable retrieving services from
IServiceProvider
while building resources. It is similar to our AddProcessor, SetSampler, AddInstrumentation, & AddReader overloads exposing the factory pattern. During public API review of 1.4 we made this internal because we weren't sure if anyone would use it. But it looks like users are interested (see: #4139).TODOs
CHANGELOG.md
updated for non-trivial changes