Skip to content

Initial ideal how to refactor provider #1008

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

Merged
cijothomas merged 11 commits into from
Aug 6, 2020
Merged

Initial ideal how to refactor provider #1008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4f1f998
initial ideal how to refactor provider
reyang Aug 5, 2020
127bc8b
dumping more ideas
reyang Aug 5, 2020
d025c22
more improvements
reyang Aug 6, 2020
c18dce3
update example
reyang Aug 6, 2020
7e83d6a
improve perf when no sampler is specified
reyang Aug 6, 2020
dc47715
put more examples
reyang Aug 6, 2020
f8bcb50
composite processor
reyang Aug 6, 2020
3db33ae
update the tutorial
reyang Aug 6, 2020
65fce94
more check
reyang Aug 6, 2020
18b78a1
nit
reyang Aug 6, 2020
0736c08
update tutorial
reyang Aug 6, 2020
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
10 changes: 7 additions & 3 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,14 +25,18 @@ public class Program

public static void Main()
{
using var otel = Sdk.CreateTracerProvider(b => b
.AddActivitySource("MyCompany.MyProduct.MyLibrary")
.UseMyExporter());
using var tracerProvider = Sdk.CreateTracerProvider(
new string[]
{
"MyCompany.MyProduct.MyLibrary",
})
.AddMyExporter();

using (var activity = MyActivitySource.StartActivity("SayHello"))
{
activity?.SetTag("foo", 1);
activity?.SetTag("bar", "Hello, World!");
activity?.SetTag("baz", new int[] { 1, 2, 3 });
}
}
}
13 changes: 4 additions & 9 deletions ...porter/TracerProviderBuilderExtensions.cs → ...-own-exporter/TracerProviderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// <copyright file="TracerProviderBuilderExtensions.cs" company="OpenTelemetry Authors">
// <copyright file="TracerProviderExtensions.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -17,15 +17,10 @@
using System;
using OpenTelemetry.Trace;

internal static class TracerProviderBuilderExtensions
internal static class TracerProviderExtensions
{
public static TracerProviderBuilder UseMyExporter(this TracerProviderBuilder builder)
public static TracerProvider AddMyExporter(this TracerProvider provider)
{
if (builder == null)
{
throw new ArgumentNullException(nameof(builder));
}

return builder.AddProcessorPipeline(p => p.SetExporter(new MyExporter()));
return provider?.AddProcessor(new SimpleActivityProcessor(new MyExporter()));
}
}
19 changes: 9 additions & 10 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 @@ -25,20 +25,19 @@ public class Program

public static void Main()
{
using var otel = Sdk.CreateTracerProvider(b => b
.AddActivitySource("MyCompany.MyProduct.MyLibrary")
using var tracerProvider = Sdk.CreateTracerProvider(
new string[]
{
"MyCompany.MyProduct.MyLibrary",
})
.AddProcessor(new MyActivityProcessor("A"))
Comment thread
reyang marked this conversation as resolved.
.AddProcessor(new MyActivityProcessor("B"));

// TODO: seems buggy as ShutdownAsync is called 6 times
// TODO: need to discuss the expectation, currently FlushAsync is not called by default
// TODO: should the dispose order be C, B, A or A, B C?
.AddProcessorPipeline(p => p.AddProcessor(current => new MyActivityProcessor("A")))
.AddProcessorPipeline(p => p.AddProcessor(current => new MyActivityProcessor("B")))
.AddProcessorPipeline(p => p.AddProcessor(current => new MyActivityProcessor("C"))));

using (var activity = MyActivitySource.StartActivity("SayHello"))
using (var activity = MyActivitySource.StartActivity("Foo"))
{
activity?.SetTag("foo", 1);
activity?.SetTag("bar", "Hello, World!");
activity?.SetTag("baz", new int[] { 1, 2, 3 });
}
}
}
12 changes: 8 additions & 4 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 @@ -16,6 +16,7 @@

using System.Diagnostics;
using OpenTelemetry;
using OpenTelemetry.Exporter.Console;
using OpenTelemetry.Trace;

public class Program
Expand All @@ -25,10 +26,13 @@ public class Program

public static void Main()
{
using var otel = Sdk.CreateTracerProvider(b => b
.AddActivitySource("MyCompany.MyProduct.MyLibrary")
.SetSampler(new MySampler()) // TODO: it is NOT working at this moment
.UseConsoleExporter());
using var tracerProvider = Sdk.CreateTracerProvider(
new string[]
{
"MyCompany.MyProduct.MyLibrary",
},
new MySampler())
.AddProcessor(new SimpleActivityProcessor(new ConsoleExporter(new ConsoleExporterOptions())));

using (var activity = MyActivitySource.StartActivity("SayHello"))
{
Expand Down
11 changes: 8 additions & 3 deletions docs/trace/getting-started/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

using System.Diagnostics;
using OpenTelemetry;
using OpenTelemetry.Exporter.Console;
using OpenTelemetry.Trace;

public class Program
Expand All @@ -25,9 +26,13 @@ public class Program

public static void Main()
{
using var otel = Sdk.CreateTracerProvider(b => b
.AddActivitySource("MyCompany.MyProduct.MyLibrary")
.UseConsoleExporter());
using var tracerProvider = Sdk.CreateTracerProvider(
new string[]
{
"MyCompany.MyProduct.MyLibrary",
});

tracerProvider.AddProcessor(new SimpleActivityProcessor(new ConsoleExporter(new ConsoleExporterOptions())));

using (var activity = MyActivitySource.StartActivity("SayHello"))
{
Expand Down
89 changes: 73 additions & 16 deletions src/OpenTelemetry/Sdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Collections.Generic;
using System.Diagnostics;
Expand All @@ -22,6 +23,7 @@
using OpenTelemetry.Context;
using OpenTelemetry.Metrics;
using OpenTelemetry.Metrics.Export;
using OpenTelemetry.Resources;
using OpenTelemetry.Trace;
using OpenTelemetry.Trace.Internal;
using OpenTelemetry.Trace.Samplers;
Expand Down Expand Up @@ -161,30 +163,85 @@ public static TracerProvider CreateTracerProvider(Action<TracerProviderBuilder>
return tracerProviderSdk;
}

public static TracerProvider CreateTracerProvider(IEnumerable<string> sources, Sampler sampler = null, Resource resource = null)
Copy link
Copy Markdown
Member

@CodeBlanch CodeBlanch Aug 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@reyang reyang Aug 6, 2020

Choose a reason for hiding this comment

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

I think it depends on what do we expect AddActivitySource to behave:

  • If the expectation is that AddActivitySource can be added at any point of the TraceProvider lifecycle, we should have it.
  • If the expectation is that AddActivitySource can only be applied while we are initializing the TraceProvider, ctor seems to the way as this is what it is designed for.

Copy link
Copy Markdown
Member Author

@reyang reyang Aug 6, 2020

Choose a reason for hiding this comment

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

@CodeBlanch as you can see from the commit history, initially I was thinking of having both AddProcessor and AddActivitySource, then I noticed:

  1. The spec wants AddProcessor to happen even after the TraceProvider is created, which wasn't working with the builder approach.
  2. The implementation didn't support changing / adding ActivitySource as this is a restriction from .NET ActivityListener (the Listener.ShoudListenTo value is cached, so subsequent updates will not take effect).

Copy link
Copy Markdown
Member

@CodeBlanch CodeBlanch Aug 6, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

@reyang reyang Aug 6, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@CodeBlanch CodeBlanch Aug 6, 2020

Choose a reason for hiding this comment

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

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.

🤷

Copy link
Copy Markdown
Member Author

@reyang reyang Aug 7, 2020

Choose a reason for hiding this comment

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

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.

{
var activitySources = new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase);

foreach (var name in sources)
{
activitySources[name] = true;
}

var provider = new TracerProviderSdk
{
Resource = resource,
Sampler = sampler,
};

provider.ActivityListener = new ActivityListener
{
// Callback when Activity is started.
ActivityStarted = (activity) =>
{
if (activity.IsAllDataRequested)
{
activity.SetResource(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) => activitySources.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, 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;

// 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)
if (sampler != null)
{
return ActivityDataRequest.AllDataAndRecorded;
// 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
Expand Down
Loading