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.

Sign up for GitHub

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

Jump to bottom

[Instrumentation.StackExchangeRedis] Use different source names for connections #2001

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[Instrumentation.StackExchangeRedis] Use different source names for connections #2001

wants to merge 10 commits into from
+113 −21

Conversation

YayBurritos
Copy link

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

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

@YayBurritos YayBurritos requested a review from a team August 8, 2024 14:29
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Aug 8, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis label Aug 8, 2024
@vishweshbankwar
Copy link
Member

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

@YayBurritos
Copy link
Author

@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
Copy link
Author

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

@Kielek Kielek changed the title #1997: Add support for instrumenting multiple redis connections [Instrumentation.StackExchangeRedis] Add support for instrumenting multiple connections Aug 27, 2024
@Kielek Kielek changed the title [Instrumentation.StackExchangeRedis] Add support for instrumenting multiple connections [Instrumentation.StackExchangeRedis] Use different source names for connections Aug 27, 2024
Kielek
Kielek requested changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I do not like the idea of this PR. The source name should be constant for the given library.
You can distinguish/decorate attributes using Enrich on the options:

Consider something like:

x.Enrich = (activity, command) => activity.AddTag("your-custom-connection-attribute-name", "your-custom-connection-name")

@matt-hensley
Copy link
Contributor

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

@YayBurritos
Copy link
Author

YayBurritos commented Sep 3, 2024 •
edited
Loading

@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.

@YayBurritos YayBurritos mentioned this pull request Sep 9, 2024
Merged
3 tasks
@Kielek
Copy link
Contributor

Kielek commented Sep 12, 2024 •
edited
Loading

@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.

@YayBurritos
Copy link
Author

@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 #1193 (comment), 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
Copy link
Contributor

Kielek commented Sep 17, 2024

@YayBurritos, any updated about MRE?

@YayBurritos
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kielek Kielek Kielek requested changes

@matt-hensley matt-hensley Awaiting requested review from matt-hensley

Requested changes must be addressed to merge this pull request.

Assignees

@matt-hensley matt-hensley

Labels
comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

StackExchangeRedis support for uniquely instrumenting multiple Redis connections
4 participants
@YayBurritos @vishweshbankwar @matt-hensley @Kielek