Initial ideal how to refactor provider#1008
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1008 +/- ##
==========================================
- Coverage 76.98% 75.56% -1.42%
==========================================
Files 220 222 +2
Lines 6087 6208 +121
==========================================
+ Hits 4686 4691 +5
- Misses 1401 1517 +116
|
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
LGTM. Will refactor rest of the code based on this. Will add tests and see if all scenarios are addressed.
Thanks!
| return tracerProviderSdk; | ||
| } | ||
|
|
||
| public static TracerProvider CreateTracerProvider(IEnumerable<string> sources, Sampler sampler = null, Resource resource = null) |
There was a problem hiding this comment.
@reyang @cijothomas Can you still call AddActivitySource(...) to add sources? If so should we make it IEnumerable<string> sources = null so you aren't forced to supply something if you want to use the extension? Personally I like the extension better but I'm all for giving users flexibility.
There was a problem hiding this comment.
I think it depends on what do we expect AddActivitySource to behave:
- If the expectation is that
AddActivitySourcecan be added at any point of theTraceProviderlifecycle, we should have it. - If the expectation is that
AddActivitySourcecan only be applied while we are initializing theTraceProvider, ctor seems to the way as this is what it is designed for.
There was a problem hiding this comment.
@CodeBlanch as you can see from the commit history, initially I was thinking of having both AddProcessor and AddActivitySource, then I noticed:
- The spec wants
AddProcessorto happen even after theTraceProvideris created, which wasn't working with the builder approach. - The implementation didn't support changing / adding
ActivitySourceas this is a restriction from .NET ActivityListener (the Listener.ShoudListenTo value is cached, so subsequent updates will not take effect).
There was a problem hiding this comment.
Sorry I'm kind of out of the loop on the pattern we're going for. If it was a builder, you create the instance. Then you build it up. At the end you call .Build and you get the final thing. So I would expect to be able to modify it up until the point build is called, after which I would expect you cannot modify it again. That help?
There was a problem hiding this comment.
@CodeBlanch correct, the builder approach works in the exact way you've describe.
Which means it doesn't give the flexibility that folks can add processor after the provider got created.
There was a problem hiding this comment.
I think we still need some kind of builder. So that exporter authors, instrumentation authors, etc., can add in "Use/Add" extensions. Those are super user friendly! In my mind, the pattern is:
using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.SetResource(ResourceTags);
.AddActivitySource("MySource")
.AddHttpClientInstrumentation()
.UseJaegerExporter()
.UseProbabilitySampler(0.5)
.Build();Build gives you a fully functioning TracerProvider aka pipeline.
The TracerProvider MAY provide methods to update the configuration.
Spec seems to say it's up to us if we want to allow the tracerProvider to be further modified. I'm impartial, but it doesn't seem unreasonable to also have support via the instance:
tracerProvider.AddProcessor(new MyProcessor());Which you could do after Build has been called.
🤷
There was a problem hiding this comment.
Totally, the way I think about this problem - we need the primitive low level APIs that are orthogonal, and user friendly APIs (e.g. builder pattern) that are expressive, make people's life easier + support other patterns such like dependency injection.
The current approach is different, it gives the higher level APIs that are not orthogonal, and make it hard for people who want to access low level functionalities, that's something I'm trying to make a change.
5 participants
@cijothomas