Skip to content
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.

Sign up for GitHub

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

Optimize the flow #1032

Closed
wants to merge 1 commit into from
Closed

Optimize the flow #1032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions docs/trace/building-your-own-exporter/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@ public class Program
public static void Main()
{
using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddActivitySources(
new string[]
{
"MyCompany.MyProduct.MyLibrary",
})
.AddActivitySource("MyCompany.MyProduct.MyLibrary")
.AddMyExporter()
.Build();

Expand Down
5 changes: 1 addition & 4 deletions docs/trace/building-your-own-processor/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,7 @@ public class Program
public static void Main()
{
using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddActivitySources(new string[]
{
"MyCompany.MyProduct.MyLibrary",
})
.AddActivitySource("MyCompany.MyProduct.MyLibrary")
.AddProcessor(new MyActivityProcessor("A"))
.AddProcessor(new MyActivityProcessor("B"))
.Build();
Expand Down
6 changes: 1 addition & 5 deletions docs/trace/building-your-own-sampler/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,7 @@ public class Program
public static void Main()
{
using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddActivitySources(
new string[]
{
"MyCompany.MyProduct.MyLibrary",
})
.AddActivitySource("MyCompany.MyProduct.MyLibrary")
.SetSampler(new MySampler())
.AddProcessor(new SimpleActivityProcessor(new ConsoleExporter(new ConsoleExporterOptions())))
.Build();
Expand Down
6 changes: 1 addition & 5 deletions docs/trace/getting-started/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,7 @@ public class Program
public static void Main()
{
using var tracerProvider = Sdk.CreateTracerProviderBuilder()
.AddActivitySources(
new string[]
{
"MyCompany.MyProduct.MyLibrary",
})
.AddActivitySource("MyCompany.MyProduct.MyLibrary")
.AddProcessor(new SimpleActivityProcessor(new ConsoleExporter(new ConsoleExporterOptions())))
.Build();

Expand Down
176 changes: 43 additions & 133 deletions src/OpenTelemetry/Trace/TracerProviderBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
// </copyright>
using System;
using System.Collections.Generic;
using System.Diagnostics;
using OpenTelemetry.Resources;
using OpenTelemetry.Trace.Samplers;

Expand All @@ -26,17 +25,19 @@ namespace OpenTelemetry.Trace
/// </summary>
public class TracerProviderBuilder
{
internal TracerProviderBuilder()
{
}
private readonly List<object> instrumentations = new List<object>();

private readonly List<ActivityProcessor> processors = new List<ActivityProcessor>();

internal Sampler Sampler { get; private set; }
private readonly List<string> sources = new List<string>();

internal Resource Resource { get; private set; } = Resource.Empty;
private Sampler sampler = new ParentOrElseSampler(new AlwaysOnSampler());

internal ActivityProcessor ActivityProcessor { get; private set; }
private Resource resource = Resource.Empty;

internal Dictionary<string, bool> ActivitySourceNames { get; private set; }
internal TracerProviderBuilder()
{
}

internal List<InstrumentationFactory> InstrumentationFactories { get; private set; }

Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double-checking, we want to allow users to set sampler to null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

return this;
}

Expand All @@ -58,45 +59,44 @@ public TracerProviderBuilder SetSampler(Sampler sampler)
/// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns>
public TracerProviderBuilder SetResource(Resource resource)
{
this.Resource = resource ?? Resource.Empty;
this.resource = resource ?? Resource.Empty;
return this;
}

/// <summary>
/// Adds given activitysource name to the list of subscribed sources.
/// </summary>
/// <param name="activitySourceName">Activity source name.</param>
/// <param name="name">Activity source name.</param>
/// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns>
public TracerProviderBuilder AddActivitySource(string activitySourceName)
{
public TracerProviderBuilder AddActivitySource(string name)
{
if (string.IsNullOrWhiteSpace(name))
{
throw new ArgumentException($"{nameof(name)} is null or whitespace.");
}

// TODO: We need to fix the listening model.
// Today it ignores version.
if (this.ActivitySourceNames == null)
{
this.ActivitySourceNames = new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase);
}
this.sources.Add(name);

this.ActivitySourceNames[activitySourceName] = true;
return this;
}

/// <summary>
/// Adds given activitysource names to the list of subscribed sources.
/// </summary>
/// <param name="activitySourceNames">Activity source names.</param>
/// <param name="names">Activity source names.</param>
/// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns>
public TracerProviderBuilder AddActivitySources(IEnumerable<string> activitySourceNames)
{
// TODO: We need to fix the listening model.
// Today it ignores version.
if (this.ActivitySourceNames == null)
{
this.ActivitySourceNames = new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase);
public TracerProviderBuilder AddActivitySource(IEnumerable<string> names)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodeBlanch mentioned it here.
This is a micro-improvement (together with AddActivitySource vs. AddSource) that can be addressed in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
if (names == null)
{
throw new ArgumentNullException(nameof(names));
}

foreach (var activitySourceName in activitySourceNames)
foreach (var name in names)
{
this.ActivitySourceNames[activitySourceName] = true;
this.AddActivitySource(name);
}

return this;
Expand All @@ -108,24 +108,14 @@ public TracerProviderBuilder AddActivitySources(IEnumerable<string> activitySour
/// <param name="activityProcessor">Activity processor to add.</param>
/// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns>
public TracerProviderBuilder AddProcessor(ActivityProcessor activityProcessor)
{
if (this.ActivityProcessor == null)
{
this.ActivityProcessor = activityProcessor;
}
else if (this.ActivityProcessor is CompositeActivityProcessor compositeProcessor)
{
compositeProcessor.AddProcessor(activityProcessor);
}
else
{
this.ActivityProcessor = new CompositeActivityProcessor(new[]
{
this.ActivityProcessor,
activityProcessor,
});
{
if (activityProcessor == null)
{
throw new ArgumentNullException(nameof(activityProcessor));
}

this.processors.Add(activityProcessor);

return this;
}

Expand Down Expand Up @@ -160,106 +150,26 @@ public TracerProviderBuilder AddInstrumentation<TInstrumentation>(

public TracerProvider Build()
{
this.Sampler = this.Sampler ?? new ParentOrElseSampler(new AlwaysOnSampler());

var provider = new TracerProviderSdk
{
Resource = this.Resource,
Sampler = this.Sampler,
ActivityProcessor = this.ActivityProcessor,
};

var activitySource = new ActivitySourceAdapter(provider.Sampler, provider.ActivityProcessor, provider.Resource);
var provider = new TracerProviderSdk(this.sources, null, this.sampler, this.resource);

foreach (var processor in this.processors)
{
provider.AddProcessor(processor);
}

var adapter = new ActivitySourceAdapter(this.sampler, provider.ActivityProcessor, this.resource);

if (this.InstrumentationFactories != null)
{
foreach (var instrumentation in this.InstrumentationFactories)
{
provider.Instrumentations.Add(instrumentation.Factory(activitySource));
this.instrumentations.Add(instrumentation.Factory(adapter));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

give it to the provider, let it deal with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i made this change in #1035

}
}

provider.ActivityListener = new ActivityListener
{
// Callback when Activity is started.
ActivityStarted = (activity) =>
{
if (activity.IsAllDataRequested)
{
activity.SetResource(this.Resource);
}

provider.ActivityProcessor?.OnStart(activity);
},

// Callback when Activity is stopped.
ActivityStopped = (activity) =>
{
provider.ActivityProcessor?.OnEnd(activity);
},

// Function which takes ActivitySource and returns true/false to indicate if it should be subscribed to
// or not.
ShouldListenTo = (activitySource) =>
{
if (this.ActivitySourceNames == null)
{
return false;
}

return this.ActivitySourceNames.ContainsKey(activitySource.Name);
},

// Setting this to true means TraceId will be always
// available in sampling callbacks and will be the actual
// traceid used, if activity ends up getting created.
AutoGenerateRootContextTraceId = true,

// This delegate informs ActivitySource about sampling decision when the parent context is an ActivityContext.
GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) => ComputeActivityDataRequest(options, this.Sampler),
};

ActivitySource.AddActivityListener(provider.ActivityListener);

return provider;
}

internal static ActivityDataRequest ComputeActivityDataRequest(
in ActivityCreationOptions<ActivityContext> options,
Sampler sampler)
{
var isRootSpan = /*TODO: Put back once AutoGenerateRootContextTraceId is removed.
options.Parent.TraceId == default ||*/ options.Parent.SpanId == default;

if (sampler != null)
{
// As we set ActivityListener.AutoGenerateRootContextTraceId = true,
// Parent.TraceId will always be the TraceId of the to-be-created Activity,
// if it get created.
ActivityTraceId traceId = options.Parent.TraceId;

var samplingParameters = new SamplingParameters(
options.Parent,
traceId,
options.Name,
options.Kind,
options.Tags,
options.Links);

var shouldSample = sampler.ShouldSample(samplingParameters);
if (shouldSample.IsSampled)
{
return ActivityDataRequest.AllDataAndRecorded;
}
}

// If it is the root span select PropagationData so the trace ID is preserved
// even if no activity of the trace is recorded (sampled per OpenTelemetry parlance).
return isRootSpan
? ActivityDataRequest.PropagationData
: ActivityDataRequest.None;
}

internal readonly struct InstrumentationFactory
{
public readonly string Name;
Expand Down
Loading