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.

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

Default Sampler update + ParentOrElse fix #1013

Merged
merged 4 commits into from
Aug 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -64,23 +64,25 @@ public override void OnStartActivity(Activity activity, object payload)
// This requires to ignore the current activity and create a new one
// using the context extracted using the format TextFormat supports.
var ctx = this.options.TextFormat.Extract(default, request, HttpRequestHeaderValuesGetter);

// Create a new activity with its parent set from the extracted context.
// This makes the new activity as a "sibling" of the activity created by
// Asp.Net.
Activity newOne = new Activity(ActivityNameByHttpInListener);
newOne.SetParentId(ctx.TraceId, ctx.SpanId, ctx.TraceFlags);
newOne.TraceStateString = ctx.TraceState;

// Starting the new activity make it the Activity.Current one.
newOne.Start();

// Both new activity and old one store the other activity
// inside them. This is required in the Stop step to
// correctly stop and restore Activity.Current.
newOne.SetCustomProperty("ActivityByAspNet", activity);
activity.SetCustomProperty("ActivityByHttpInListener", newOne);
activity = newOne;
if (ctx != default)
{
// Create a new activity with its parent set from the extracted context.
// This makes the new activity as a "sibling" of the activity created by
// Asp.Net.
Activity newOne = new Activity(ActivityNameByHttpInListener);
newOne.SetParentId(ctx.TraceId, ctx.SpanId, ctx.TraceFlags);
newOne.TraceStateString = ctx.TraceState;

// Starting the new activity make it the Activity.Current one.
newOne.Start();

// Both new activity and old one store the other activity
// inside them. This is required in the Stop step to
// correctly stop and restore Activity.Current.
newOne.SetCustomProperty("ActivityByAspNet", activity);
activity.SetCustomProperty("ActivityByHttpInListener", newOne);
activity = newOne;
}
}

// see the spec https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,19 @@ public override void OnStartActivity(Activity activity, object payload)
// using the format TextFormat supports.

var ctx = this.options.TextFormat.Extract(default, request, HttpRequestHeaderValuesGetter);

// Create a new activity with its parent set from the extracted context.
// This makes the new activity as a "sibling" of the activity created by
// Asp.Net Core.
Activity newOne = new Activity(ActivityNameByHttpInListener);
newOne.SetParentId(ctx.TraceId, ctx.SpanId, ctx.TraceFlags);
newOne.TraceStateString = ctx.TraceState;

// Starting the new activity make it the Activity.Current one.
newOne.Start();
activity = newOne;
if (ctx != default)
{
// Create a new activity with its parent set from the extracted context.
// This makes the new activity as a "sibling" of the activity created by
// Asp.Net Core.
Activity newOne = new Activity(ActivityNameByHttpInListener);
newOne.SetParentId(ctx.TraceId, ctx.SpanId, ctx.TraceFlags);
newOne.TraceStateString = ctx.TraceState;

// Starting the new activity make it the Activity.Current one.
newOne.Start();
activity = newOne;
}
}

activity.SetKind(ActivityKind.Server);
Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
disposes the containing exporter.
* `BroadcastActivityProcessor`is disposable and it disposes the processors.
* Samplers now get the actual TraceId of the Activity to be created.
* Default Sampler changed from AlwaysOn to ParentOrElse(AlwaysOn) to match the
spec.

## 0.3.0-beta

Expand Down
7 changes: 4 additions & 3 deletions src/OpenTelemetry/Sdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public static MeterProvider CreateMeterProvider(Action<MeterBuilder> configure)
meterRegistry,
metricProcessor,
metricExporter,
meterBuilder.MetricPushInterval == default(TimeSpan) ? DefaultPushInterval : meterBuilder.MetricPushInterval,
meterBuilder.MetricPushInterval == default ? DefaultPushInterval : meterBuilder.MetricPushInterval,
cancellationTokenSource);

var meterProviderSdk = new MeterProviderSdk(metricProcessor, meterRegistry, controller, cancellationTokenSource);
Expand All @@ -85,7 +85,7 @@ public static TracerProvider CreateTracerProvider(Action<TracerProviderBuilder>
configureTracerProviderBuilder?.Invoke(tracerProviderBuilder);

var tracerProviderSdk = new TracerProviderSdk();
Sampler sampler = tracerProviderBuilder.Sampler ?? new AlwaysOnSampler();
Sampler sampler = tracerProviderBuilder.Sampler ?? new ParentOrElseSampler(new AlwaysOnSampler());

ActivityProcessor activityProcessor;
if (tracerProviderBuilder.ProcessingPipelines == null || !tracerProviderBuilder.ProcessingPipelines.Any())
Expand Down Expand Up @@ -165,7 +165,8 @@ internal static ActivityDataRequest ComputeActivityDataRequest(
in ActivityCreationOptions<ActivityContext> options,
Sampler sampler)
{
var isRootSpan = options.Parent.SpanId == default;
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,
Expand Down
25 changes: 11 additions & 14 deletions src/OpenTelemetry/Trace/ActivitySourceAdapter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,23 +76,20 @@ private void RunGetRequestedData(Activity activity)
ActivityContext parentContext;
if (string.IsNullOrEmpty(activity.ParentId))
{
parentContext = default(ActivityContext);
parentContext = default;
}
else if (activity.Parent != null)
{
parentContext = activity.Parent.Context;
}
else
{
if (activity.Parent != null)
{
parentContext = activity.Parent.Context;
}
else
{
parentContext = new ActivityContext(
activity.TraceId,
activity.ParentSpanId,
activity.ActivityTraceFlags,
activity.TraceStateString,
isRemote: true);
}
parentContext = new ActivityContext(
activity.TraceId,
activity.ParentSpanId,
activity.ActivityTraceFlags,
activity.TraceStateString,
isRemote: true);
}

var samplingParameters = new SamplingParameters(
Expand Down
3 changes: 2 additions & 1 deletion src/OpenTelemetry/Trace/Samplers/ParentOrElseSampler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ public ParentOrElseSampler(Sampler delegateSampler)
public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
{
var parentContext = samplingParameters.ParentContext;
if (parentContext == default)
if (/* TODO: TraceId is always provided due to AutoGenerateRootContextTraceId. That is being removed in RC1 and this can be put back.
parentContext.TraceId == default ||*/ parentContext.SpanId == default)
{
// If no parent, use the delegate to determine sampling.
return this.delegateSampler.ShouldSample(samplingParameters);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
using Moq;
using OpenTelemetry.Instrumentation.Grpc.Tests.Services;
using OpenTelemetry.Trace;
using OpenTelemetry.Trace.Samplers;
using Xunit;

namespace OpenTelemetry.Instrumentation.Grpc.Tests
Expand All @@ -44,6 +45,7 @@ public void GrpcClientCallsAreCollectedSuccessfully(string baseAddress)

using (Sdk.CreateTracerProvider(
(builder) => builder
.SetSampler(new AlwaysOnSampler())
.AddGrpcClientInstrumentation()
.SetResource(expectedResource)
.AddProcessorPipeline(p => p.AddProcessor(n => spanProcessor.Object))))
Expand Down Expand Up @@ -95,6 +97,7 @@ public void GrpcAndHttpClientInstrumentationIsInvoked()

using (Sdk.CreateTracerProvider(
(builder) => builder
.SetSampler(new AlwaysOnSampler())
.AddHttpClientInstrumentation()
.AddGrpcClientInstrumentation()
.AddProcessorPipeline(p => p.AddProcessor(n => spanProcessor.Object))))
Expand Down