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.

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

Proposal: Add Datadog Tracing Support for rueidis #3031

Open
korECM opened this issue Dec 12, 2024 · 3 comments
Open

Proposal: Add Datadog Tracing Support for rueidis #3031

korECM opened this issue Dec 12, 2024 · 3 comments
Labels
apm:ecosystem contrib/* related feature requests or bugs proposal/accepted Accepted proposals proposal more in depth change that requires full team approval

Comments

@korECM
Copy link
Contributor

korECM commented Dec 12, 2024

The rueidis library currently supports OpenTelemetry tracing via rueidisotel and provides a hook mechanism through rueidishook.
I propose adding native Datadog tracing support to enhance observability for users of the rueidis Redis client.

@korECM korECM added the enhancement quick change/addition that does not need full team approval label Dec 12, 2024
@github-actions github-actions bot added the needs-triage New issues that have not yet been triaged label Dec 12, 2024
@rarguelloF
Copy link
Contributor

Hello! 👋

This sounds like a great addition, so please consider this proposal accepted.

PRs are more than welcome, but if you want us to tackle the work feel free to reach out Datadog Support so we prioritize accordingly.

@rarguelloF rarguelloF added proposal/accepted Accepted proposals proposal more in depth change that requires full team approval apm:ecosystem contrib/* related feature requests or bugs and removed needs-triage New issues that have not yet been triaged enhancement quick change/addition that does not need full team approval labels Dec 20, 2024
@korECM
Copy link
Contributor Author

korECM commented Dec 22, 2024

@rarguelloF
Great! I'll take care of this task!

@korECM
Copy link
Contributor Author

korECM commented Dec 22, 2024

@rarguelloF

I have some concerns while working on Redis integration.

Unlike go-redis, rueidis doesn't provide a way to inspect Client options once a Client is created. This suggests we should implement the integration using the trace library's NewClient consistently rather than using a WrapClient approach. I'd like to get thoughts on whether this approach makes sense.

// go-redis

// Options returns read-only Options that were used to create the client.
func (c *Client) Options() *Options {
	return c.opt
}

// rueidis

// Client is the redis client interface for both single redis instance and redis cluster. It should be created from the NewClient()
type Client interface {
	CoreClient

	DoCache(ctx context.Context, cmd Cacheable, ttl time.Duration) (resp RedisResult)

	DoMultiCache(ctx context.Context, multi ...CacheableTTL) (resp []RedisResult)

	DoStream(ctx context.Context, cmd Completed) RedisResultStream

	DoMultiStream(ctx context.Context, multi ...Completed) MultiRedisResultStream

	Dedicated(fn func(DedicatedClient) error) (err error)

	Dedicate() (client DedicatedClient, cancel func())

	Nodes() map[string]Client
}

Another concern is about determining which Client type (Single, Sentinel, Cluster) will be created from given Options, which is needed for setting Redis span tags. While we can examine the related logic in rueidis' internal code, there are two main concerns:

  1. Is it appropriate for the trace library to depend on the internal logic of rueidis? I'm worried this creates too tight a coupling with the library's internal implementation.
  2. Even if we decide to depend on the internal logic, there's a complication: rueidis attempts to create a Cluster Client first, and falls back to Single Client if that fails. This makes it impossible to infer which Client type will be created just by examining the Options. I'm wondering what would be the best approach in this situation.

@korECM korECM changed the title Proposal: Add Datadog Tracing Support for Rueidis Proposal: Add Datadog Tracing Support for rueidis Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs proposal/accepted Accepted proposals proposal more in depth change that requires full team approval
Projects
None yet
Development

No branches or pull requests

2 participants