fix(redisotel): fix the situation of reporting spans multiple times

[flc1125]

[ndyakov]

Hello, @flc1125, thanks for contributing. Would you mind describing the issue that you are trying to fix in more details:

• Why was this hook not needed?
• Why was it introduced initially?
• Is there a possibility this change breaks something?

And please add some tests if possible.

[flc1125]

Hello, @flc1125, thanks for contributing. Would you mind describing the issue that you are trying to fix in more details:

• Why was this hook not needed?
• Why was it introduced initially?
• Is there a possibility this change breaks something?

And please add some tests if possible.

OK, I am on vacation, I will deal with this issue when I have free time.

[flc1125]

@ndyakov When I was solving this problem, I found that some dependent packages would be automatically upgraded. That is to say, this PR, see if it can be prioritized. #3265

[flc1125]

Additionally, as a supplementary inquiry, I would like to understand the following information:

1. Currently, both the Go version and the dependency versions in this package are quite outdated. Do we have any requirements or limitations regarding the minimum supported version of Go? For example, upgrading to 1.22 or higher.

2. Can we use the following in the project: github.com/stretchr/testify

[ndyakov]

Additionally, as a supplementary inquiry, I would like to understand the following information:

1. Currently, both the Go version and the dependency versions in this package are quite outdated. Do we have any requirements or limitations regarding the minimum supported version of Go? For example, upgrading to 1.22 or higher.

I am on the same boat as you and still trying to figure out if we have any blockers to upgrade to a newer Go version. I do think @monkey92t or @vmihailenco can share if they need this library to keep 1.18 as a minimum go version.

2. Can we use the following in the project: github.com/stretchr/testify

No need to add another assert library, we do use a fork of https://onsi.github.io/gomega/ and I hope to soon actually have the official https://onsi.github.io/gomega/ . Both questions are kind of related, if we are fine to update the go version we may try to move to the official https://onsi.github.io/gomega/ and https://onsi.github.io/ginkgo/ libraries.

[ndyakov]

@ndyakov When I was solving this problem, I found that some dependent packages would be automatically upgraded. That is to say, this PR, see if it can be prioritized. #3265

I still don't understand how and why this issue occurred. Does removing the hooks break anything else or is it a safe fix. Again, maybe @monkey92t has more experience with the otel codebaase in go-redis.

[flc1125]

@ndyakov When I was solving this problem, I found that some dependent packages would be automatically upgraded. That is to say, this PR, see if it can be prioritized. #3265

I still don't understand how and why this issue occurred. Does removing the hooks break anything else or is it a safe fix. Again, maybe @monkey92t has more experience with the otel codebaase in go-redis.

That's not the case, it's just an additional optimization. You can ignore this issue. 😄

[flc1125]

• Why was this hook not needed?

In the cluster architecture, when a Redis command is initiated, it will generate two spans of the link due to the repeated Hook. But I think one span should be enough actually.

• Why was it introduced initially?

I think I can't answer this question, maybe need to consult the author at that time.

This is an early PR: #2244

@vmihailenco Please help, and see if you can answer this question.

• Is there a possibility this change breaks something?

Since it cancelled a link, it is a disruptive change. But I think it might be a defect rather than a function. It should be fixed.

---

After the above conclusions are drawn, I will combine the results to conduct supplementary tests.

[flc1125]

Is there any latest news?

Can I just add unit tests according to my current implementation?

[monkey92t]

can share if they need this library to keep 1.18 as a minimum go version.

There don't seem to be any issues preventing the Go version upgrade. It currently remains at Go 1.18, likely just due to a lack of maintenance.

In the cluster architecture, when a Redis command is initiated, it will generate two spans of the link due to the repeated Hook. But I think one span should be enough actually.

I don't seem to see any unnecessary hooks in the PR.

[flc1125]

I don't seem to see any unnecessary hooks in the PR.

When I initiate a command like GET, there are two Span nodes, parent and child, in the link. Is there any reason for this? My expectation is that one would be sufficient, and the other seems redundant.

[flc1125]

An example:

package go_redis_otel

import (
	"context"
	"testing"

	"github.com/redis/go-redis/extra/redisotel/v9"
	"github.com/redis/go-redis/v9"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"go.opentelemetry.io/otel/sdk/trace"
	"go.opentelemetry.io/otel/sdk/trace/tracetest"
)

var ctx = context.Background()

func TestRedis(t *testing.T) {
	client := redis.NewClusterClient(&redis.ClusterOptions{
		Addrs: []string{"localhost:7000", "localhost:7001", "localhost:7002"},
	})

	imsb := tracetest.NewInMemoryExporter()
	provider := trace.NewTracerProvider(trace.WithSyncer(imsb))

	require.NoError(t, redisotel.InstrumentTracing(client, redisotel.WithTracerProvider(provider)))

	cmd := client.Ping(ctx)
	require.NoError(t, cmd.Err())

	spans := imsb.GetSpans()
	pingCount := 0
	for _, span := range spans {
		t.Logf("Span: Name: %s, SpanKind: %v, TraceID: %s, SpanID: %s, ParentSpanID: %s \n",
			span.Name, span.SpanKind, span.SpanContext.TraceID().String(), span.SpanContext.SpanID().String(), span.Parent.SpanID().String())

		if span.Name == "ping" {
			pingCount++
		}
	}
	assert.Equal(t, 2, pingCount, "expected 2 ping spans")
}

output:

=== RUN   TestRedis
    redis_test.go:34: Span: Name: redis.dial, SpanKind: client, TraceID: 9a4f1fb48699d791e63889d8b0c41cd4, SpanID: 5108423ff6984071, ParentSpanID: 72a5559f1570ee5e 
    redis_test.go:34: Span: Name: cluster slots, SpanKind: client, TraceID: 9a4f1fb48699d791e63889d8b0c41cd4, SpanID: 72a5559f1570ee5e, ParentSpanID: 2445a984671aa2d5 
    redis_test.go:34: Span: Name: redis.dial, SpanKind: client, TraceID: 9a4f1fb48699d791e63889d8b0c41cd4, SpanID: 0178803be166df66, ParentSpanID: e598802d78ea422a 
    redis_test.go:34: Span: Name: ping, SpanKind: client, TraceID: 9a4f1fb48699d791e63889d8b0c41cd4, SpanID: e598802d78ea422a, ParentSpanID: 2445a984671aa2d5 
    redis_test.go:34: Span: Name: ping, SpanKind: client, TraceID: 9a4f1fb48699d791e63889d8b0c41cd4, SpanID: 2445a984671aa2d5, ParentSpanID: 0000000000000000 
--- PASS: TestRedis (0.00s)
PASS

Among them, Span: Name: ping appears twice.

[monkey92t]

It looks correct. We have already added a hook for each newly created node in OnNewNode. We should remove the redundant hooks. Good!

[monkey92t]

ClusterClient contains multiple Client instances and has a global hook that is executed for any command. Each Client also has its own hook, but in Cluster mode, the hook is only executed when the command is routed to the corresponding Client node. The ClusterClient hook is indeed redundant, and we should remove it.

cc @ndyakov Please approve this PR, it requires permission.

[flc1125]

If the function meets expectations, I will improve the unit test before entering the review stage.

[ndyakov]

@monkey92t approved

[monkey92t]

@flc1125 Do you need to add more tests? I won’t specifically require tests for tracing since it’s not a core feature of go-redis.

If you don’t need to add more tests, we can finalize the PR and proceed with the merge.

[flc1125]

If you don’t need to add more tests, we can finalize the PR and proceed with the merge.

I will add that if it is necessary to expedite the merge process, this merge can be carried out. I can submit another PR later for additional updates.

PS: I have been quite busy recently, so the progress might be affected.

[monkey92t]

If you don’t need to add more tests, we can finalize the PR and proceed with the merge.

I will add that if it is necessary to expedite the merge process, this merge can be carried out. I can submit another PR later for additional updates.

PS: I have been quite busy recently, so the progress might be affected.

Alright, you can add some tests when you have time. Thanks for your PR!

[monkey92t]

@ndyakov Can this be merged?