Provide db.system attribute when activity is created for use with sampling

[johnduhart]

I'm currently trying to implement a custom sampler that drops database spans that do not have a parent span. Many of our services occasionally make database calls outside of trace, and this produces a bunch of noisy single-span traces that aren't very interesting. We were able to implement this sampling logic easily for our Ruby and Java services, but I'm running into trouble implementing this for .NET.

In order to determine if a given span is a "database span", we've been relying on checking if the db.system attribute is present. Sadly, most .NET instrumentation that I've seen so far defers adding this tag until after sampling has occurred.

Examples:

• SQL Client
• EF Core
• Redis
• Elasticsearch

The API specification says the following:

Whenever possible, users SHOULD set any already known attributes at span creation instead of calling SetAttribute later.

I appreciate the desire to minimize performance impact on non-sampled call path, but is it possible to strike a balance? Can we define a set of attributes that should always be provided, and treat that as a best practice for when writing instrumentation?

Workaround

This is the solution I've had to implement for my use case in the meantime:

internal sealed class DbSampler : Sampler
{
    private static readonly HashSet<string> DatabaseSpanNames = new(new[]
    {
        "OpenTelemetry.SqlClient.Execute",
        "OpenTelemetry.EntityFrameworkCore.Execute"
    });
    
    private readonly Sampler _innerSampler;

    public DbSampler(Sampler innerSampler)
    {
        _innerSampler = innerSampler;
    }

    public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
    {
        if (samplingParameters.ParentContext == default
            && DatabaseSpanNames.Contains(samplingParameters.Name))
        {
            return new SamplingResult(SamplingDecision.Drop);
        }
        
        return _innerSampler.ShouldSample(samplingParameters);
    }
}

There's a few drawbacks here, namely that I have to now maintain a static list of all possible names a database span can be created with. This solution also does not work for Redis, which creates an activity using the command name as the activity name.

If the activity source was passed along with the SamplingParameters (as described in this specification issue), that would solve the latter problem.

[cijothomas]

@johnduhart If instrumentation is NOT populating already known attributes to the StartActivity call, then it can addressed in the instrumentation itself. There is a cost of allocating ActivityTagsCollection when calling StartActivity and this was the original reason for not doing it. I think it would be reasonable to ask .NET to provide StartActivity overload which accepts tag1,tag2,tag3 (upto 3 or 5) to avoid this alloc for small number of initial tags. This would help instrumentation provide the known tags to StartActivity call, without incurring the alloc when using ActivityTagsCollection

[cijothomas]

@johnduhart Btw - for the SqlClient instrumentation, sampler can see the displayname, which should tell that its a SQL call. Will that work for you?

[cijothomas]

@johnduhart Btw - for the SqlClient instrumentation, sampler can see the displayname, which should tell that its a SQL call. Will that work for you?

Never mind. I just read your full issue description now.

[johnduhart]

If instrumentation is NOT populating already known attributes to the StartActivity call, then it can addressed in the instrumentation itself.

I'd be happy to open a PR myself to add this, I'm just unsure of where the line for "already known" should be drawn. Is it only things that can be kept in a statically allocated tags collection (eg. db.system for the SQL client instrumentation), or can we leverage information accessible via property fetchers?

[Oberon00]

sampler can see the displayname, which should tell that its a SQL call

I think using a "display name" for anything other than displaying to a human reader sounds like bad practice.

[cijothomas]

sampler can see the displayname, which should tell that its a SQL call

I think using a "display name" for anything other than displaying to a human reader sounds like bad practice.

DisplayName in Activity is the Span.Name, which is one of the inputs to sampler. So samplers are allowed to make decisions based on it, right? (though name can change later..)

[cijothomas]

If instrumentation is NOT populating already known attributes to the StartActivity call, then it can addressed in the instrumentation itself.

I'd be happy to open a PR myself to add this, I'm just unsure of where the line for "already known" should be drawn. Is it only things that can be kept in a statically allocated tags collection (eg. db.system for the SQL client instrumentation), or can we leverage information accessible via property fetchers?

I think, the only thing which is already available and can be provided to StartActivity call is the db.System attribute, as its not required to be parsed from any payloads. Anything which needs to parsed out using fetchers would be a good candidate to do after sampling decision is done.

It seems correct if we can statically create tags with db.System, and provide it to StartActivity call. This should allow samplers to access db.System, and does not cause perf issues either.