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

fix: limit the bytes of redis args #153

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

rfyiamcool
Copy link
Contributor

summary

limit the bytes of redis args.

@@ -187,3 +187,13 @@ func getKey(args []interface{}) string {
}
return key
}

const maxArgsLength = 1024
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard code the limitation? I don't think this is a good practice.

Please follow this pattern, https://github.com/apache/skywalking-go/blob/main/tools/go-agent/config/agent.default.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, please cr again 😁

@wu-sheng wu-sheng requested a review from mrproliu November 29, 2023 16:46
Signed-off-by: rfyiamcool <rfyiamcool@163.com>
@rfyiamcool rfyiamcool force-pushed the fix/limit_bytes_redis_args branch from d1f15f3 to 80e9655 Compare November 30, 2023 03:13
@@ -4,5 +4,6 @@
|--------------------------------|-------------------------------------------------------|---------------|----------------------------------------------------------------|
| http.server_collect_parameters | SW_AGENT_PLUGIN_CONFIG_HTTP_SERVER_COLLECT_PARAMETERS | false | Collect the parameters of the HTTP request on the server side. |
| mongo.collect_statement | SW_AGENT_PLUGIN_CONFIG_MONGO_COLLECT_STATEMENT | false | Collect the statement of the MongoDB request. |
| redis.max_args_bytes | SW_AGENT_PLUGIN_CONFIG_REDIS_MAX_ARGS_BYTES | 1024 | Limit the bytes size of redis args request. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align the order here and config file.
This should be after sql config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. 😅

sorry, because of the display problem of the ide, I didn't notice that the table was not aligned.

@mrproliu
Copy link
Contributor

Could you add this feature into the CHANGES.md?

Signed-off-by: rfyiamcool <rfyiamcool@163.com>
@rfyiamcool
Copy link
Contributor Author

Could you add this feature into the CHANGES.md?

done. 😁

@mrproliu
Copy link
Contributor

Please fix the CI.

@rfyiamcool
Copy link
Contributor Author

Please fix the CI.

I don't know how to fix this problem. 😅 give me some Pointers.

@wu-sheng
Copy link
Member

CI had logs, you should be able to read them, and CI is able to run locally.

@wu-sheng
Copy link
Member

Did you test the codes you changed? It seems it doesn't work and runnable actually.

https://github.com/apache/skywalking-go/actions/runs/7044082174/job/19171487594?pr=153

Signed-off-by: rfyiamcool <rfyiamcool@163.com>
@rfyiamcool rfyiamcool force-pushed the fix/limit_bytes_redis_args branch from c916a1d to 63e4de5 Compare December 1, 2023 07:57
Copy link
Contributor

@mrproliu mrproliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@mrproliu mrproliu merged commit 99c2c38 into apache:main Dec 1, 2023
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants