Auto instrumentation for go-redis #505
Conversation
| @@ -6,5 +6,5 @@ | |||
| if test -z "$(go env GOMOD)"; then | |||
| pwd | |||
| else | |||
| find -type f -name go.mod \! -path './vendor/*' -printf "%h\n" | |||
| find . -type f -name go.mod \! -path './vendor/*' -exec dirname '{}' \; | |||
There was a problem hiding this comment.
this is just a fix to be not-GNU compatible
|
@aspacca I've just merged a change into master which fixes the test failure in https://travis-ci.org/elastic/apm-agent-go/jobs/520141217. You'll need to rebase or merge master to get that. There are some other CI failures due to formatting, e.g. https://travis-ci.org/elastic/apm-agent-go/jobs/520141216. You can fix that with Finally, you will need to sign the CLA before I can merge this. |
Codecov Report
@@ Coverage Diff @@
## master #505 +/- ##
==========================================
+ Coverage 84.08% 84.09% +0.01%
==========================================
Files 113 114 +1
Lines 6674 6729 +55
==========================================
+ Hits 5612 5659 +47
- Misses 758 766 +8
Partials 304 304
Continue to review full report at Codecov.
|
|
@axw everything should be fine now |
|
Thanks @aspacca. I'm away on vacation at the moment, I will finish reviewing next week. |
|
|
||
| // Wrap wraps client such that executed commands are reported as spans to Elastic APM, | ||
| // using the client's associated context. | ||
| // A context-specific client may be obtained by using Client.WithContext. |
There was a problem hiding this comment.
It appears from the tests that not everything works with Ring. Can you please add a comment here explaining what isn't supported, linking to issues if there are any?
There was a problem hiding this comment.
Ring has not TxPipeline/TxPipelined implemented, it's not related to the instrumentation with the apm agent, do you want me to add a comment for this?
While, related to instrumentation, it works with Ring on Pipeline/Pipelined but not for direct command on the client: this PR redis/go-redis#1017 will address
For me it's ok to wait for that PR to be merged, and then remove the special case in the test here
There was a problem hiding this comment.
I see, thanks. I'd prefer to wait for redis/go-redis#1017 to merge first, to avoid a little bit of churn.
|
redis/go-redis#1017 merged, pushed final commit here |
|
I ran the docker tests locally, and I'm seeing this error occur in the integration tests: Any ideas what's going on there? |
|
redis cluster was not setup properly: all the cluster nodes must be up before we start the cluster. with docker compose 2.2 you can set healthcheck to be sure that containers dependencies are satisfied upon readiness and not only bootstrap. but you cannot do anymore in version 3 (that’s used by the apm-agent-go). I tried to overcome the issue setting a restart “on_failure” for the redis cluster container but you may have hit a corner case where this is not enough
if the cluster is not setup properly in general go-redis cluster client will fails
|
This is the case, but unrelated to timing. Logs from redis-cluster: Looks like an issue with the entrypoint commant. I'll have to wait until I have some time to debug this before merging, as I don't want local testing to be blocked. |
|
it’s really strange because it passed jenkins build, unless they pushed a new redis:latest with different behavior in the meanwhile
|
|
the case could be also that you don’t have updated redis:latest image :)
|
I tried that already, it wasn't it. The EDIT: maybe it was a stale image after all. I might've failed to recreate the service. It seems to be working now. I'll run some more tests and merge it once I'm confident it's not going to recur. |
|
Looks like it was me after all, I've run it a bunch of times without any failures. |
|
Thanks again for your work on this, @aspacca. |
addresses #489