[Instrumentation.StackExchangeRedis] Use different source names for connections

YayBurritos commented 3 months ago

Fixes #1997

Changes

Use existing name parameter of TracerProviderBuilderExtensions -> AddRedisInstrumentation to create a unique ActivitySource for the Redis connection.

Instrumenting multiple Redis connections

using OpenTelemetry.Trace;

public class Program
{
    public static void Main(string[] args)
    {
        // Connect to the first server.
        var firstConnectionName = "myRedisConnection1";
        using var connection1 = ConnectionMultiplexer.Connect("server1:6379");

        // Connect to the second server.
        var secondConnectionName = "myRedisConnection2";
        using var connection2 = ConnectionMultiplexer.Connect("server2:6379");

        using var tracerProvider = Sdk.CreateTracerProviderBuilder()
            .AddRedisInstrumentation(firstConnectionName, connection1, null)
            .AddRedisInstrumentation(secondConnectionName, connection2, null)
            .AddConsoleExporter()
            .Build();
    }
}

If name isn't provided, the prior (default) behavior will be respected.

Default Behavior

using OpenTelemetry.Trace;

public class Program
{
    public static void Main(string[] args)
    {
        // Connect to the server.
        using var connection = ConnectionMultiplexer.Connect("localhost:6379");

        using var tracerProvider = Sdk.CreateTracerProviderBuilder()
            .AddRedisInstrumentation(connection)
            .AddConsoleExporter()
            .Build();
    }
}

Merge requirement checklist

[x] CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
[x] Unit tests added/updated
[ ] Appropriate CHANGELOG.md files updated for non-trivial changes
[ ] Changes in public API reviewed (if applicable)

linux-foundation-easycla[bot] commented 3 months ago

The committers listed above are authorized under a signed CLA.

:white_check_mark: login: YayBurritos (fa5fc460ebd5cd73ed1fc4771a73f275b26950f8, 74f9f46fdf05fa3d9a18a496032a0a2a1464c12a, ddcb55d3199cb18e039c1c1053fbc3112589bf3a, 268d7469c45db1445ddb495aa1c7aa33698a8818, 131b67355959131992f637738479173ba3d6b1bf, 857a62f536ac4f5683e4e5cdb0ebe6aefc1ea3d6, efe14d0165c73e379dfe206f6fd9facc0ddb9394, 6b8b396fb5d90d176ab122cd771741ba47709fd6, b8517156283f5997c4fcb3b612a1e264340a2e49, 945c7baf1b0cbcc22ab4623591f60d46044ba1a1)

vishweshbankwar commented 3 months ago

@YayBurritos - Please sign the CLA so that the PR can be reviewed.

YayBurritos commented 3 months ago

@YayBurritos - Please sign the CLA so that the PR can be reviewed.

@vishweshbankwar : Yes, I'm working on getting the signature from my client. We had some folks out of the office last week. I apologize for the delay on this. I'm anxious to keep this moving along!

YayBurritos commented 2 months ago

@vishweshbankwar, @matt-hensley: FYI - CLA has been signed. Thanks for your patience!

matt-hensley commented 2 months ago

Agree with @Kielek, dynamic ActivitySource naming is unusual. I believe the use case can be handled by named configurations /connections.

YayBurritos commented 2 months ago

@Kielek , @matt-hensley : Totally understand your concerns on this PR. The issue I'm trying to address is duplicate activities/spans being created when you attempt to instrument different Redis connection multiplexers. Enriching doesn't solve this problem.

In our testing, if we have Connection A and Connection B, and there's an HMGET call for Connection A, we see two spans for the HMGET (since there are two multiplexers that are instrumented) rather than one. This is the behavior I'm trying to address here.

This solution (while it may be "unusual") simply provides an option to instrument the connections independently - with the main goal of avoiding duplicate span creation. Users can continue to do things the old/standard way if they prefer. This is simply an additional option that can be used (not a required approach for anyone).

Also, regarding ActivitySource naming (from https://opentelemetry.io/docs/languages/net/instrumentation/#setting-up-an-activitysource)...

It’s generally recommended to define ActivitySource once per app/service that is been instrumented, but you can instantiate several ActivitySources if that suits your scenario.

If there's a better solution, I'm open to that so feel free to provide recommendations. But the library as-is seems to have an issue for our particular scenario.

Kielek commented 2 months ago

@YayBurritos, Did you have tested construction from https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1193#issue-1712724215 - Usage section?

You should not call AddRedisInstrumentation twice. There is a dedicated way to add additional connection to the instrumentation.

EDIT: If it does not help, could you please provide Minimal, Reproducible Example. There is a chance that we will be able to advice better solution for managing the connections.

YayBurritos commented 2 months ago

@YayBurritos, Did you have tested construction from #1193 (comment) - Usage section?

You should not call AddRedisInstrumentation twice. There is a dedicated way to add additional connection to the instrumentation.

EDIT: If it does not help, could you please provide Minimal, Reproducible Example. There is a chance that we will be able to advice better solution for managing the connections.

Hi @Kielek - Thanks for the reply! I did try the approach shown in https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1193#issue-1712724215, but in my testing, that resulted in no Redis-related spans being produced. I was quite surprised by that result.

I can look to provide an MRE. That will help me better understand if there's something odd/unusual with our application, or if we still see the same behavior with the library.

Kielek commented 2 months ago

@YayBurritos, any updated about MRE?

YayBurritos commented 2 months ago

@YayBurritos, any updated about MRE?

HI @Kielek! I don't yet have an MRE to share, but I have been working on some reorg within our code and I may (still need to do more testing) have resolved our duplicate span issue without requiring any changes to the Instrumentation.StackExchangeRedis package. Our issue may be the result of chaining an Exporter each time we call AddRedisInstrumentation.

Again, still need to test a bit to prove that theory, but that's the path I'm heading down at the moment. Will let you know when I have some results.

YayBurritos commented 1 month ago

@Kielek : Upon further testing, it does appear that our duplicate span issue for Redis-related activity was the result of our overzealous use of exporters. I updated our application to only use one exporter (chained after the first instrumented Redis connection), but continue to call AddRedisInstrumentation for each of the other connections we use. Results show what appear to be the "correct" spans (spans associated with a single Redis connection, rather than duplicates). As such, I think I'm good if we close this PR as well as the related issue. Thanks for the review and feedback on this!

Kielek commented 1 month ago

Great to hear that. Feel free to create issues or prs if you see any place for improvements. Especially PRs are.more than welcomed :)

open-telemetry / opentelemetry-dotnet-contrib