open-telemetry / opentelemetry-dotnet

The OpenTelemetry .NET Client
https://opentelemetry.io
Apache License 2.0
3.23k stars 765 forks source link

Testing BaseProcessor based filters #5771

Open devedse opened 3 months ago

devedse commented 3 months ago

What is the question?

Hello all, I've written a filter to filter out SQL Queries taking too long. I'm writing a few more of these and thus want to also test if this works.

    public sealed class SqlFilteringProcessor : BaseProcessor<Activity>
    {
        public override void OnEnd(Activity activity)
        {
            if (activity.Source.Name == "OpenTelemetry.Instrumentation.SqlClient" && activity.Status != ActivityStatusCode.Error && activity.Duration < TimeSpan.FromSeconds(5))
            {
                activity.ActivityTraceFlags &= ~ActivityTraceFlags.Recorded;
            }
        }
    }
}

My plan was to simply deserialize a whole Activity stored as JSON and pass it into this processor. However the Activity class can't be deserialized because it doesn't have setter for most properties.

Is there another way I'd be able to test my logic without having to write a wrapper around the Activity class and increasing the complexity in a number of areas.

Additional context

N/a

cijothomas commented 3 months ago

My plan was to simply deserialize a whole Activity stored as JSON and pass it into this processor

Why is that needed? Activity passed to the onend has all the information one needs to make a filtering decision. What is it missing?

devedse commented 3 months ago

I'd like to be able to test with mocked activities, but I can not simply set all properties. E.g. I want to create a mocked activity with a duration of 5 seconds, an Operation name of X and something else. Not all properties are simple data fields; hence can't be set by me or a JsonDeserializer.

cijothomas commented 3 months ago

Please list all the things you want to set on Activity and I can check if it is directly supported or via alternates. Activity is also the public user facing API, so if you cannot set something on it for testing, then users will have same issue! Once you give a list, I can help further.

an Operation name of X

Activity's OperationName is a legacy thing, and has no use in OpenTelemetry. Only DisplayName is required for OTel uses. (It is the Span Name from OTel spec). You can set it at Activity creation time or anytime afterwards.

devedse commented 3 months ago

Well these are some of the processors I want to test:

    internal sealed class HealthCheckFilteringProcessor : BaseProcessor<Activity>
    {
        public override void OnEnd(Activity activity)
        {
            if (activity.OperationName == "Microsoft.AspNetCore.Hosting.HttpRequestIn" && activity.Tags.Any(t => t.Key == "url.path" && (t.Value == "/api/ping" || t.Value == "/api/health")) && activity.Status != ActivityStatusCode.Error && activity.Duration < TimeSpan.FromSeconds(5))
            {
                activity.ActivityTraceFlags &= ~ActivityTraceFlags.Recorded;
            }
        }
    }
    internal sealed class KeyVaultFilteringProcessor : BaseProcessor<Activity>
    {
        public override void OnEnd(Activity activity)
        {
            if ((activity.Source.Name == "Azure.Security.KeyVault.Secrets.SecretClient" || activity.Parent?.Source.Name == "Azure.Security.KeyVault.Secrets.SecretClient") && activity.Status != ActivityStatusCode.Error && activity.Duration < TimeSpan.FromSeconds(5))
            {
                activity.ActivityTraceFlags &= ~ActivityTraceFlags.Recorded;
            }
        }
    }

So I need:

cijothomas commented 3 months ago

Activity.Source.Name Activity.Parent.Source.Name Activity.Status Activity.Duration Activity.Tags Activity.OperationName (I guess I could replace this by: "DisplayName": "GET",)

All of these can be provided while using the ActivitySource.StartActivityAPI/its overloads and via SetTag, SetStatus.. What is missing?