Skip to content

feat(redisotel): add WithSkipSpanIfNotRecording option#3765

Open
thani-ath-nain wants to merge 3 commits intoredis:masterfrom
thani-ath-nain:feat/skips-span-if-not-recording
Open

feat(redisotel): add WithSkipSpanIfNotRecording option#3765
thani-ath-nain wants to merge 3 commits intoredis:masterfrom
thani-ath-nain:feat/skips-span-if-not-recording

Conversation

@thani-ath-nain
Copy link
Copy Markdown

@thani-ath-nain thani-ath-nain commented Apr 5, 2026

This PR was motivated by #3760 and addresses it by adding an option to skip span recording, which makes the change backward compatible.


Note

Low Risk
Low risk and opt-in: only changes tracing behavior when WithSkipSpanIfNotRecording(true) is set, reducing unnecessary span creation when tracing is disabled or sampled out.

Overview
Adds a new tracing option, WithSkipSpanIfNotRecording, that prevents redisotel hooks from starting redis.dial, command, and pipeline spans when the span in the incoming context is not recording.

Includes comprehensive tests covering dial/process/pipeline behavior for no parent span, sampled-out parents, and preserving the existing default behavior when the option is disabled.

Reviewed by Cursor Bugbot for commit c6910b2. Bugbot is set up for automated code reviews on this repo. Configure here.

@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented Apr 5, 2026

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

@thani-ath-nain
Copy link
Copy Markdown
Author

@Jesse-Bonfire @wzy9607 @htemelski-oss can you please review this PR

Comment thread extra/redisotel/tracing.go
@thani-ath-nain
Copy link
Copy Markdown
Author

CI failing due to code that is unrelated to my changes:

=== RUN   TestDialConn_DialTimeoutDisabled_DoesNotSetDeadline
    dial_conn_retry_test.go:174: expected 1 dial attempt, got 2
--- FAIL: TestDialConn_DialTimeoutDisabled_DoesNotSetDeadline (0.00s)

@ndyakov
Copy link
Copy Markdown
Member

ndyakov commented Apr 7, 2026

@thani-ath-nain, we do have two flaky tests that are the reason for the failing CI. I am aware of those

ndyakov
ndyakov previously approved these changes Apr 7, 2026
Copy link
Copy Markdown
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

Code looks good to me, will let the community decide if we would like to merge this.

@thani-ath-nain
Copy link
Copy Markdown
Author

Addressed Cursor’s review by applying WithSkipSpanIfNotRecording to DialHook too and added tests. Verified with go test ./... in extra/redisotel the remaining CI issue is still the known unrelated flaky test. Can a maintainer please review and merge this?

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit c6910b2. Configure here.

Comment thread extra/redisotel/config.go
@thani-ath-nain
Copy link
Copy Markdown
Author

@ndyakov Thank you for the review! I appreciate you acknowledging the flaky test issue. Since the code has your approval, would you be able to tag a few other maintainers who might have context on the tracing/telemetry parts of go-redis? I'd love to get additional eyes on this to help the community decide. Alternatively, if there are specific concerns or discussion points that need to be addressed before merging, I'm happy to provide more context or make adjustments. The feature is backward-compatible (opt-in only) and addresses a real performance concern; happy to elaborate on the use case if helpful.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants