Add an integration for GitHub.com/redis/rueidis#1098
Conversation
|
Hey there! Anything we can do to move this forward? It's a bit disappointing to not even get a reply from New Relic. |
|
Hey @ankon Sorry about the delay. I'll review it now. Do you mind rebasing to our develop branch? |
ankon
force-pushed
the
feature/rueidis-integration
branch
from
d66e08c to
a23e504
Compare
|
hey @mirackara, I rebased it onto |
ankon
force-pushed
the
feature/rueidis-integration
branch
2 times, most recently
from
c2c9f35 to
af78430
Compare
|
Hey @ankon, thanks! Do you mind also switching to our develop branch? otherwise this is looking good! I've also approved our workflows |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1098 +/- ##
===========================================
- Coverage 78.99% 78.71% -0.29%
===========================================
Files 159 160 +1
Lines 15074 15144 +70
===========================================
+ Hits 11908 11920 +12
- Misses 2751 2809 +58
Partials 415 415 see 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This makes the dev-shell actually usable on SELinux-enabled environments.
ankon
force-pushed
the
feature/rueidis-integration
branch
from
af78430 to
49d7a97
Compare
|
|
||
| volumes: | ||
| - ${PWD}:/usr/src/app/go-agent | ||
| - ${PWD}:/usr/src/app/go-agent:rw,Z |
There was a problem hiding this comment.
(nit): volume mounts are read-write by default. we could simplify these lines to just use :Z
| } | ||
|
|
||
| func (h *hook) DoCache(client rueidis.Client, ctx context.Context, cmd rueidis.Cacheable, ttl time.Duration) (resp rueidis.RedisResult) { | ||
| // XXX: Is it always true that there is only one command here? |
There was a problem hiding this comment.
In rueidis, calling .Commands() returns a slice of strings where index 0 is always the command name. I believe this is true and a safe way to get the command -- same for line 52. We can remove these comments
|
|
||
| func (h *hook) DoStream(client rueidis.Client, ctx context.Context, cmd rueidis.Completed) rueidis.RedisResultStream { | ||
| // XXX: Is it always true that there is only one command here? | ||
| // XXX: Should we mark this as "streaming" |
There was a problem hiding this comment.
I think marking this as streaming is a good idea. we can clear these up too
| } | ||
|
|
||
| // Parse the first init address to get host and port. | ||
| // XXX: Does rueidis support unix sockets? |
There was a problem hiding this comment.
APM dashboard won't be able to show where the Redis queries are going whenever a Unix socket is used. net.SplitHostPort only looks for port seperators. We could add a check before we call SplitHostPort here
addr := opt.InitAddress[0]
// Check if the address looks like a Unix domain socket path
if strings.HasPrefix(addr, "/") || strings.HasSuffix(addr, ".sock") {
h.segment.Host = "localhost"
h.segment.PortPathOrID = addr // New Relic allows paths/IDs in this field
} else {
// Fall back to standard TCP parsing if it's not a Unix socket
if host, port, err := net.SplitHostPort(addr); err == nil {
......
}
There was a problem hiding this comment.
Since rueidis requires a live Redis instance to execute integration tests against, should we spin up a standard redis image inside this docker-compose.yaml?
3 participants
Links
see #983: we have been using this code in production for quite some time now.
Details
The first commit fixes the dev-shell to work on Fedora Linux. It should be a no-op on other (non-SELinux-enabled) systems.