Optimize the flow #1032
Optimize the flow #1032
Conversation
|
|
||
| if (this.InstrumentationFactories != null) | ||
| { | ||
| foreach (var instrumentation in this.InstrumentationFactories) | ||
| { | ||
| provider.Instrumentations.Add(instrumentation.Factory(activitySource)); | ||
| this.instrumentations.Add(instrumentation.Factory(adapter)); |
There was a problem hiding this comment.
give it to the provider, let it deal with it.
| @@ -47,7 +48,7 @@ internal TracerProviderBuilder() | |||
| /// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns> | |||
| public TracerProviderBuilder SetSampler(Sampler sampler) | |||
| { | |||
| this.Sampler = sampler ?? throw new ArgumentNullException(nameof(sampler)); | |||
| this.sampler = sampler; | |||
There was a problem hiding this comment.
Double-checking, we want to allow users to set sampler to null?
There was a problem hiding this comment.
At least for now as we're seeing big memory allocation/perf difference here.
With sampler == null, allocation is 656 bytes.
With sampler != null, allocation is 1248 bytes.
Once we solved the allocation issue in sampler, we can explore if we want to add the validation back (and remove all the sampler? logic from the SDK).
| if (this.ActivitySourceNames == null) | ||
| { | ||
| this.ActivitySourceNames = new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase); | ||
| public TracerProviderBuilder AddActivitySource(IEnumerable<string> names) |
There was a problem hiding this comment.
Advantages/disadvantages to make this:
public TracerProviderBuilder AddActivitySource(params string[] names)I guess one disadvantage would be that the use of .ToArray() may be required.
There was a problem hiding this comment.
@CodeBlanch mentioned it here.
This is a micro-improvement (together with AddActivitySource vs. AddSource) that can be addressed in a separate PR.
There was a problem hiding this comment.
It's too bad C# doesn't let us do params IEnumerable<string> eh? Personally in this case I would do params string[] because this is initialization code. I feel like users will prefer the convenience and it's unlikely they'll have an IEnumerable<string> from some other source in the init phase they'll want to pass us. The 99% case is it's a fixed list set during init. In more general APIs, probably the IEnumerable<string> is better.
There was a problem hiding this comment.
Ah, yes, I thought I had seen this suggestion, and thinking "yea, I like this" 😄. Agreed separate PR makes sense, and also agreed the context of this being initialization code makes params seem natural here.
|
#1035 supercedes this as it already was branched off of this branch from Reiley. |
Changes
TracerProviderSdkclass, builder will only validate the input and callBuild()(partially done).AddActivitySources.Please provide a brief description of the changes here. Update the
CHANGELOG.mdfor non-trivial changes.For significant contributions please make sure you have completed the following items: