Create span when SamplingDecision.Drop #3841

alanwest · 2022-10-28T19:43:09Z

Create spans even on a sampling decision of drop. This conforms the .NET SDK to the specification regarding span creation.

Generate a new span ID for the Span, independently of the sampling decision. This is done so other components (such as logs or exception handling) can rely on a unique span ID, even if the Span is a non-recording instance.

This may have a performance impact as an Activity will always be allocated irrespective of sampling decision. Notable exceptions are the ASP.NET Core and HttpClient instrumentation. The Activity created by these libraries was already always allocated.

cijothomas · 2022-10-28T19:47:58Z

Time to really comply with the spec 🤣. I vaguely remember some discussions around this in some SIG calls, and we still decided to use the current approach.... Maybe the heap allocation was a bigger concern?

alanwest · 2022-10-28T19:55:08Z

Maybe the heap allocation was a bigger concern?

Yes! Was just talking to @CodeBlanch about this. Allocations gonna be painful (note the name of my branch 😆)

But this may be a hit we'll need to take... it's important for both propagating context properly and also correlation of trace data to logs and metrics.

cijothomas · 2022-10-28T19:56:27Z

If a root span (t1, s1) is created.. then its child gets not created, and then someone makes a log. the log will be parented to the (t1,s1).

With this change, the child span gets created (t1, s2, parent=s1), but wont see processor or exporter. The log will now be parented to t1,s2.. which might be an issue as log is now pointing to a parent, who is nowhere to be found....

cijothomas · 2022-10-28T19:59:14Z

If a root span (t1, s1) is created.. then its child gets not created, and then someone makes a log. the log will be parented to the (t1,s1).

With this change, the child span gets created (t1, s2, parent=s1), but wont see processor or exporter. The log will now be parented to t1,s2.. which might be an issue as log is now pointing to a parent, who is nowhere to be found....

unless we make log point to the span which is sampled or root. (like walk the .Parent chain until we run out of parent)...

alanwest · 2022-10-28T19:59:45Z

The log will now be parented to t1,s2.. which might be an issue as log is now pointing to a parent, who is nowhere to be found....

My understanding is that this is by design. That is, incomplete traces are to be expected as well as telemetry correlated to things that happened but were not recorded.

alanwest · 2022-10-28T20:01:38Z

unless we make log point to the span which is sampled or root. (like walk the .Parent chain until we run out of parent)...

If walking the chain is required, then I think this needs to be clarified by the spec.

CodeBlanch · 2022-10-28T20:08:12Z

A couple tags...

#3290

#3828 (comment)

alanwest · 2022-11-02T21:59:57Z

Closing for now... as discussed in this week's SIG, we do want to make this change, but we want to wait until after the 1.4.0 release. I will pursue fixing up the tests and open the PR back up later.

TimothyMothra · 2023-02-16T00:28:09Z

FYI; tested locally and confirmed this change would fix the issue reported in #4087

cijothomas · 2023-02-16T01:08:24Z

@TimothyMothra Thanks for investigating it more! This is tagged to resurrected for 1.5. Please chime in with a +1 to the original issue.

cijothomas

LGTM. Lets add a good writeup in the PR description about the potential impact this has, and a changelog entry.

Highlighting that for a asp.net core + httpclient scenario, the impact is nil, as these libraries would anyway create activities, so no impact.
Higher allocation, for the case when there are more than span in the trace within proc..

cijothomas · 2023-02-25T05:43:14Z

@Oberon00
Could you review please?

codecov · 2023-02-25T07:07:08Z

Codecov Report

Merging #3841 (61b4dd4) into main (a3ec494) will decrease coverage by 0.09%.
The diff coverage is 75.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3841      +/-   ##
==========================================
- Coverage   85.71%   85.62%   -0.09%     
==========================================
  Files         289      289              
  Lines       11374    11341      -33     
==========================================
- Hits         9749     9711      -38     
- Misses       1625     1630       +5

Impacted Files	Coverage Δ
...tryProtocol/Implementation/MetricItemExtensions.cs	`93.33% <0.00%> (-0.63%)`	⬇️
...metryProtocol/Implementation/ActivityExtensions.cs	`95.90% <100.00%> (-0.31%)`	⬇️
src/OpenTelemetry/Trace/TracerProviderSdk.cs	`99.26% <100.00%> (-0.02%)`	⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs	`56.25% <0.00%> (-6.25%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`76.47% <0.00%> (-5.89%)`	⬇️
...lementation/SqlClientInstrumentationEventSource.cs	`70.83% <0.00%> (-4.17%)`	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	`97.14% <0.00%> (-2.86%)`	⬇️
...tp/Implementation/HttpHandlerDiagnosticListener.cs	`73.19% <0.00%> (-1.04%)`	⬇️
...p/Implementation/HttpInstrumentationEventSource.cs	`76.00% <0.00%> (+4.00%)`	⬆️

Oberon00 · 2023-02-27T09:46:52Z

src/OpenTelemetry/Trace/TracerProviderSdk.cs

@@ -240,7 +239,7 @@ internal sealed class TracerProviderSdk : TracerProvider
 else if (this.sampler is AlwaysOffSampler)
 {
 listener.Sample = (ref ActivityCreationOptions<ActivityContext> options) =>
- !Sdk.SuppressInstrumentation ? PropagateOrIgnoreData(options.Parent) : ActivitySamplingResult.None;
+ !Sdk.SuppressInstrumentation ? ActivitySamplingResult.PropagationData : ActivitySamplingResult.None;


This is extremely hard to read. There is some deep nesting of if/else/Listener callback code going on here. I would suggest to extract some (named, top-level) methods to make the code more readable/reviewable.

Oberon00 · 2023-02-27T10:04:13Z

Maybe only tangentially related but:

I wonder how one would implement transparent API-only propagation in .NET, where you just echo through extracted incoming span contexts: https:/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#behavior-of-the-api-in-the-absence-of-an-installed-sdk

reyang · 2023-02-27T16:52:49Z

@alanwest an Activity is fairly expensive from the garbage collection point of view. Could we explore if it's possible to defer the creation only when some code is reading it? Alternatively, maybe explore if a slim version of Activity can be used/reused rather than a full version.

Oberon00 · 2023-02-27T16:56:56Z

an Activity is fairly expensive from the garbage collection point of view

Maybe System.Diagnostics.Activity is a bad match for the OpenTelemetry Span interface, since I guess the OTel spec was written imagining that a non-recording Span can be implemented rather efficiently.

alanwest · 2023-02-28T22:11:40Z

Could we explore if it's possible to defer the creation only when some code is reading it? Alternatively, maybe explore if a slim version of Activity can be used/reused rather than a full version.

I think solving this issue within the opentelemetry-dotnet project may prove challenging. Either deferring creation or implementing a slim version may require instrumentation authors to depend on OpenTelemetry.Api rather than just System.Diagnosics to retrieve current span context. I think this is something we're aiming to minimize.

Activity is fairly expensive from the garbage collection point of view

Talking with @CodeBlanch and trying to brainstorm options we might suggest to .NET team to make Activity itself lighter weight. Though, I guess the first question is whether the size of Activity is the primary concern.

In other words, is Activity expensive from a GC point of view because of its size? Or is it expensive irrespective of size and we really just want to avoid heap allocations altogether?

cijothomas · 2023-03-01T15:37:30Z

Maybe only tangentially related but:

I wonder how one would implement transparent API-only propagation in .NET, where you just echo through extracted incoming span contexts: https:/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#behavior-of-the-api-in-the-absence-of-an-installed-sdk

This might be difficult/impossible.
One can extract ActivityContext using Propagators, but it does not have a place to put them in (so that it can be further injected to downstream calls)...unless an Activity itself is created with the extracted Context...

It might be possible to do just the baggage part, as once can use Propagators to extract it, store in Baggage.Current, and it can be injected to downstream calls as well.

cijothomas · 2023-03-01T15:40:49Z

Could we explore if it's possible to defer the creation only when some code is reading it? Alternatively, maybe explore if a slim version of Activity can be used/reused rather than a full version.

I think solving this issue within the opentelemetry-dotnet project may prove challenging. Either deferring creation or implementing a slim version may require instrumentation authors to depend on OpenTelemetry.Api rather than just System.Diagnosics to retrieve current span context. I think this is something we're aiming to minimize.

Activity is fairly expensive from the garbage collection point of view

Talking with @CodeBlanch and trying to brainstorm options we might suggest to .NET team to make Activity itself lighter weight. Though, I guess the first question is whether the size of Activity is the primary concern.

In other words, is Activity expensive from a GC point of view because of its size? Or is it expensive irrespective of size and we really just want to avoid heap allocations altogether?

https:/open-telemetry/opentelemetry-dotnet/blob/main/test/Benchmarks/Trace/TraceBenchmarks.cs#L32 Its 416 B per activity minimum without any tags. Adding tags will make it ~650B/activity.

cijothomas · 2023-03-01T15:41:57Z

may require instrumentation authors to depend on OpenTelemetry.Api rather than just System.Diagnosics to retrieve current span context

Something like ActivityContext.Current (like Baggage.Current) which can be thought of as just the context part, and can be valid/used even if Activity.Current is null?

alanwest · 2023-03-01T20:12:13Z

Something like ActivityContext.Current (like Baggage.Current) which can be thought of as just the context part, and can be valid/used even if Activity.Current is null?

The trouble is, I'm not sure how something like this would work. Consider the following:

using (var span1 = source.StartActivity("SampledSpan"))
{
    using (var span2 = source.StartActivity("NotSampledSpan"))
    {
        // span2 is null here, but let's imagine that ActivityContext.Current = span2's context
        
        // logs and exemplars are correlated to span2's context
        log.Info("Some log message.");
        histogram.Record(100);

        // span2's context is propagated
        httpClient.SendAsync(...);
    }

    // How do we that the scope of span2 has ended?
    // We'd need something like ActivityContext.Current.Stop(), but how would this get invoked?
}

alanwest · 2023-03-01T20:21:24Z

My sense is that we're stuck with working within the Activity API as is and optimizing perf here will require collaborating with the .NET team. Could ActivitySource introduce an object pool?

alanwest · 2023-03-01T20:37:54Z

Its 416 B per activity minimum without any tags.

This could probably be reduced by doing something like:

class Activity
{
   // This would be null if this Activity was not sampled
   private ActivityData Data;
   
   // This would be a valid context
   private ActivityContext context;
}

class ActivityData
{
    // Move all the fields on Activity today to this class
    // basically the fields here: https:/dotnet/runtime/blob/0a15c3b97dabaf94e5d4399aef2fd48c2b3f446a/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L90-L116
}

But I'm not sure this helps address the primary concern. Activities would still be allocated and require garbage collection. We'd need to explore if reducing the size of Activity reduces the overhead of garbage collection.

github-actions · 2023-03-09T03:18:44Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2023-03-17T03:17:08Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Create span when SamplingDecision.Drop

98cc2ac

alanwest closed this Nov 2, 2022

alanwest reopened this Feb 24, 2023

cijothomas reviewed Feb 24, 2023

View reviewed changes

alanwest added 4 commits February 24, 2023 17:29

Merge branch 'main' into alanwest/allocate-more-spans-please

02ae442

Modifiy tests to match new behavior

8ee9dcf

Update changelog

383afc8

Merge branch 'main' into alanwest/allocate-more-spans-please

81b56f1

alanwest marked this pull request as ready for review February 25, 2023 02:16

alanwest requested a review from a team February 25, 2023 02:16

Fix test. Parent's span id is no longer propagated.

61b4dd4

Oberon00 reviewed Feb 27, 2023

View reviewed changes

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 9, 2023

github-actions bot closed this Mar 17, 2023

Oberon00 mentioned this pull request Jun 13, 2023

Framework for defining the maturity of OpenTelemetry and its SIGs open-telemetry/oteps#232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create span when SamplingDecision.Drop #3841

Create span when SamplingDecision.Drop #3841

alanwest commented Oct 28, 2022 •

edited

Loading

cijothomas commented Oct 28, 2022

alanwest commented Oct 28, 2022

cijothomas commented Oct 28, 2022

cijothomas commented Oct 28, 2022

alanwest commented Oct 28, 2022

alanwest commented Oct 28, 2022

CodeBlanch commented Oct 28, 2022

alanwest commented Nov 2, 2022

TimothyMothra commented Feb 16, 2023

cijothomas commented Feb 16, 2023

cijothomas left a comment •

edited

Loading

cijothomas commented Feb 25, 2023

codecov bot commented Feb 25, 2023

Oberon00 Feb 27, 2023

Oberon00 commented Feb 27, 2023

reyang commented Feb 27, 2023

Oberon00 commented Feb 27, 2023 •

edited

Loading

alanwest commented Feb 28, 2023

cijothomas commented Mar 1, 2023

cijothomas commented Mar 1, 2023

cijothomas commented Mar 1, 2023

alanwest commented Mar 1, 2023 •

edited

Loading

alanwest commented Mar 1, 2023

alanwest commented Mar 1, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 17, 2023

Create span when SamplingDecision.Drop #3841

Create span when SamplingDecision.Drop #3841

Conversation

alanwest commented Oct 28, 2022 • edited Loading

cijothomas commented Oct 28, 2022

alanwest commented Oct 28, 2022

cijothomas commented Oct 28, 2022

cijothomas commented Oct 28, 2022

alanwest commented Oct 28, 2022

alanwest commented Oct 28, 2022

CodeBlanch commented Oct 28, 2022

alanwest commented Nov 2, 2022

TimothyMothra commented Feb 16, 2023

cijothomas commented Feb 16, 2023

cijothomas left a comment • edited Loading

Choose a reason for hiding this comment

cijothomas commented Feb 25, 2023

codecov bot commented Feb 25, 2023

Codecov Report

Oberon00 Feb 27, 2023

Choose a reason for hiding this comment

Oberon00 commented Feb 27, 2023

reyang commented Feb 27, 2023

Oberon00 commented Feb 27, 2023 • edited Loading

alanwest commented Feb 28, 2023

cijothomas commented Mar 1, 2023

cijothomas commented Mar 1, 2023

cijothomas commented Mar 1, 2023

alanwest commented Mar 1, 2023 • edited Loading

alanwest commented Mar 1, 2023

alanwest commented Mar 1, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 17, 2023

alanwest commented Oct 28, 2022 •

edited

Loading

cijothomas left a comment •

edited

Loading

Oberon00 commented Feb 27, 2023 •

edited

Loading

alanwest commented Mar 1, 2023 •

edited

Loading