Fix redis cluster, ring cluster lead err msg. Add Redis Unit Test #86
Conversation
…ad err. "skyWalking failed to create exit span, got error: parameter are nil"
|
You need to keep one PR, please don't re-create it. |
|
The CI failed, please check. |
|
This is my first PR. |
|
Please focus on this CI: https://github.com/apache/skywalking-go/actions/runs/5759830947/job/15614675582?pr=86 |
|
I need create redis service containers in workflows. |
|
Then, I think you should mock the data when check the data in the UT. |
| http.HandleFunc("/execute", executeHandler) | ||
|
|
||
| http.HandleFunc("/health", func(writer http.ResponseWriter, request *http.Request) { | ||
| writer.WriteHeader(http.StatusOK) |
There was a problem hiding this comment.
Is this needed?I believe if you don't do anything it will also return 200.
There was a problem hiding this comment.
I noticed that there is a "/health" route written in each plugin file. This is probably used to check the health status of the service, right?
There was a problem hiding this comment.
totally, what I am saying is that you don't need to write 200, it will by default.
There was a problem hiding this comment.
I understand. If I don't register this "/health" route, it indeed returns a 200 status code.I will delete this route register.
There was a problem hiding this comment.
@JamesSunXX Please don't delete the /health route, we need to access it to check the application has been set up success. @jcchavezs means you don't need to write writer.WriteHeader(http.StatusOK)(only this line).
| - { key: cache.args, value: 'command: map[]' } | ||
| - segmentId: not null | ||
| spans: | ||
| - operationName: redis/cluster slots |
There was a problem hiding this comment.
Please don't add the blank in the operation name, they should separate by '/'. Also, why do there have so many no-entry segments (only exit span)?
There was a problem hiding this comment.
redis command cluster slots is a full operationName. I will handle this action.I find this original PR contributor only use CreateExitSpan handle redis hook.
| - { key: cache.args, value: not null } | ||
| - segmentId: not null | ||
| spans: | ||
| - operationName: GET:/health |
There was a problem hiding this comment.
We don't need to verify the 'GET:/health' segment, please delete it.
There was a problem hiding this comment.
If I delete GET:/health segment, it will validate actual data failed.
There was a problem hiding this comment.
It would not. The tool would only check declared, not the exact match.
| componentId: 5014 | ||
| isError: false | ||
| spanType: Exit | ||
| peer: not null |
There was a problem hiding this comment.
Could you please provide the peer address?
There was a problem hiding this comment.
The image bitnami/redis-cluster provides environment variables REDIS_CLUSTER_ANNOUNCE_IP , but after trying many times, I couldn't successfully initialize the cluster redis with the specified IP. Also, the Go service couldn't access redis.
There was a problem hiding this comment.
My intention is not the IP address, after testing it should be the address filled in the program. Isn't it?
|
Looks like the CI failure, please re-check. |
|
docker-compose doesn't support boolean data types. In this commit, I fix it. |
|
We had cost a lot of resources on the CI process. Why didn't you run the tests and make sure it could pass locally? |
|
I did run the tests locally and it pass.Should I delete this commit change about the tests? |
|
You should pass the plugin tests locally, not just test the codes manually. All steps on CI are runnable locally. Your failures show your plugin test codes are not verified by yourself. |
|
I suggest a few things here:
- only run tests on the plugin that changed.
- adds go cache to cache test results
- add ephimeral cache to artifacts that are being downloaded
- review the usage of `always()` as it will run steps even if previous
failed
- verify if you have prerequisites in jobs so you don't run something that
you know on beforehand it will fail.
…On Thu, 10 Aug 2023, 05:00 吴晟 Wu Sheng, ***@***.***> wrote:
You should pass the plugin tests locally, not just test the codes manually.
All steps on CI are runnable locally.
Your failures show your plugin test codes are not verified by yourself.
You made us to run those dozens of times, I don't think it makes sense.
—
Reply to this email directly, view it on GitHub
<#86 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAQFAQHMBDGVXEBWBILXURFEVANCNFSM6AAAAAA3D2ZWEU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hi, @jcchavezs . Thanks for your advice.
That sounds good. But there maybe have some plugin dependencies another plugin, such as
We don't have so many files that need to be downloaded, so could be ignored.
We already using the
We need the person creating PRs to test the CI locally in order to save time. |
Please follow the GHA file to test locally. For example, the lint errors should still be prompted on your local machine. |
|
Yeah ideally the person writing the PR should verify everything in local. What I am seeking here is to be defensive in the way we incur in CI costs. About running tests in local. Would it be a good idea to have a pre-commit hook? This would trigger tests before commit which is handy and automated. |
|
(base) ➜ plugins git:(main) ✗ ./run.sh go-redisv9
|
|
So, why did you upload the plugin test log? The plugin test code is there, your need to deep look at it why it returns |
Do all commits made by the user in their own branch automatically trigger CI? Will this result in a significant consumption of GHA resources and frequent receipt of CI emails from GitHub Actions for the user? |
For now, it wouldn't. But we can't see the result of CI, so does the contributor.
It costs the slots in the whole ASF org, which is why INFRA disabled the auto-running. This is not the reason I am asking. The issue is, we have to approve the CI to run, in order to see whether the tests passed. But every time, it fails. I can't see the point of doing this. |
Nope. A pre-commit hooks runs locally, run all targets you decide in local before the commit can be made in local. See https://pre-commit.com/ |
pre-hook is local, but with the increment of the plugins(look at Java agent plugin list), every commit will become a pain. |
The information above consists of actual collected data, and the code logic below is from testing. Why does the pipeline operation on the Redis cluster appear in two different segmentIds? Shouldn't this log data be within the same segmentId? |
|
This depends on whether your interception point is in the same goroutine (or goroutine created by the current HTTP handler) as the current consumer's access address. If the interception point is in a different goroutine, it will be impossible to determine their correlation. |
|
In the cluster redis, the hook in the |
|
Please follow this documentation section to operate tracing context. |
|
There is no update for one week. |
c.AddHook(newRedisHook(""))in cluster client, ring client may lead err msg: "go-redis :skyWalking failed to create exit span, got error: parameter are nil"