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

[Redis Receiver] Rename to generic name #33672

Closed
julianocosta89 opened this issue Jun 20, 2024 · 11 comments
Closed

[Redis Receiver] Rename to generic name #33672

julianocosta89 opened this issue Jun 20, 2024 · 11 comments
Labels
receiver/redis Redis related issues

Comments

@julianocosta89
Copy link
Member

julianocosta89 commented Jun 20, 2024

Component(s)

receiver/redis

Describe the issue you're reporting

Currently the Redis receiver is called Redis (as expected 😅), but Redis has recently changed its licensing distribution, and Valkey was created by the community to continue the project as an Open Source driven project.
Valkey/about:

The transition garnered substantial backing from leading tech corporations including AWS, Google, Oracle, Ericsson, and Snap, with the Linux Foundation stepping in to provide a stable platform for this new direction.

Linux Foundation Launches Open Source Valkey Community.

It would be great to rename the component to a more generic name, as Valkey works perfectly with it.

I've just sent a PR to the OTel demo replacing Redis with Valkey: open-telemetry/opentelemetry-demo#1619

@rogercoll suggested datastore, but anything more generic than the Redis name, would be great.

Not sure how we would do this migration without introducing a breaking change though.

@julianocosta89 julianocosta89 added the needs triage New item requiring triage label Jun 20, 2024
@github-actions github-actions bot added the receiver/redis Redis related issues label Jun 20, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@puckpuck
Copy link

What about cachestore to more accurately represent what type of datastore it is.

@rogercoll
Copy link
Contributor

Not sure how we would do this migration without introducing a breaking change though.

Although I would prefer to rename it to a more generic name, I am afraid we will have breaking changes in the configuration if in the future we try to add more cachestores other than redis and valkey.

Another solution would be to move the redis receiver logic into the internal collector's pkg and create a new valkey receiver.

@puckpuck
Copy link

puckpuck commented Jun 20, 2024

How much should we care about breaking changes on an upgrade in the demo?

helm upgrade otel-demo ... is not guaranteed to work correctly because the init-containers may return successfully before the dependent service is upgraded. You don't "upgrade" a docker compose setup.

@atoulme
Copy link
Contributor

atoulme commented Jun 21, 2024

The migration would consist of creating a new receiver with the new name, potentially copying code over. We would then deprecate the old receiver and remove it after a period of 6 months or so.

The change would be pervasive because you'd also want to look at all the metric names and descriptions and update them as part of it.

@julianocosta89
Copy link
Member Author

How much should we care about breaking changes on an upgrade in the demo?

helm upgrade otel-demo ... is not guaranteed to work correctly because the init-containers may return successfully before the dependent service is upgraded. You don't "upgrade" a docker compose setup.

@puckpuck I don't think the issue is the OTel demo here, but everyone else using the RedisReceiver.

@andrzej-stencel
Copy link
Member

Redis and Valkey are now two separate products, even if Valkey was forked from Redis at some point. It's not impossible that the implementations of their metrics endpoints will diverge at some point.

We should leave the Redis receiver as is and create a separate Valkey receiver, potentially re-using the code from the current Redis receiver.

@andrzej-stencel andrzej-stencel removed the needs triage New item requiring triage label Jun 26, 2024
@rogercoll
Copy link
Contributor

@julianocosta89 @puckpuck We discussed this topic in yesterday's SIG and, as Andrzej pointed out, we think that the best solution would be creating a new valkey receiver. As mentioned, they are two different products that might diverge in the future and having a generic cachestore receiver would need to support any future/other cachestore, thus needing to adopt a new breaking configuration. In my opinion, is the same scenaio as having a podman and a docker receiver instead of a container one.

New component's proposal: #33787

Please feel free to add any context or leave a comment on the issue if you would like to contribute to the new receiver as well.

@hughesjj
Copy link
Contributor

@rogercoll can we close this issue out then given the proposal 33787 seems to be tracking this ask?

@rogercoll
Copy link
Contributor

@rogercoll can we close this issue out then given the proposal #33787 seems to be tracking this ask?

sgtm, what do you think @julianocosta89 ?

@julianocosta89
Copy link
Member Author

also good from my end.
thank you 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
receiver/redis Redis related issues
Projects
None yet
Development

No branches or pull requests

6 participants