-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add a 'canceled' metrics attribute to separate context errors #3566
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
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
c528798 to
5eaacc5
Compare
|
Hello, @pvragov and thank you for your contribution. Will review next week. |
|
p.s. the name |
It could be tricky to define the list of error types and support it. I think there are two ways to solve this problem we should discuss.
p.s I used the 'canceled' attribute to solve the problem so as not to create a new metric and not break the backward compatibility and I completely agree that it's not the best solution of the problem. |
|
@pvragov I am fine with having attribute here, but why should it be a boolean one? Can't it be a string ? Even if we decide to only cover the |
Yes, it can. What about an 'result' attribute: success, fail, canceled, timeout ? p.s. Technically |
|
Network tcp timeout won’t map to the timeout that you are parsing at the
moment. If there is no error, success makes sense to me as a value.
Nedyalko Dyakov
On Tue, 28 Oct 2025 at 18:32, pvragov ***@***.***> wrote:
*pvragov* left a comment (redis/go-redis#3566)
<#3566 (comment)>
@pvragov <https://github.com/pvragov> I am fine with having attribute
here, but why should it be a boolean one? Can't it be a string ? Even if we
decide to only cover the context errors let's have more information about
the type. Having it named canceled when it includes timeout errors as
well sounds incorrect in my mind.
Yes, it can. What about an 'result' attribute: success, fail, canceled,
timeout ?
p.s. Technically timeout means cancelation by timeout and there is no
problem to name this case as 'canceled'
—
Reply to this email directly, view it on GitHub
<#3566 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALZXMSSIZLOZSGFGHXRBGD3Z6LDHAVCNFSM6AAAAACKF7JMUKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTINJXGQYDINJZGE>
.
You are receiving this because you commented.Message ID:
***@***.***>
This email and any attachments may contain confidential and privileged information intended only for the recipient. If you are not the intended recipient, please notify the sender immediately, delete this email, and refrain from disclosing or using its contents. Unauthorized use, disclosure, or distribution of this communication is prohibited. Should you have any questions, please contact ***@***.***
|
|
The main idea is separate context errors from others. As for me is enough to have one attribute 'result': success, fail, canceled and use it instead of state attribute. The 'state' attribute could be deprecated. It's ok to mark network timeouts by the 'fail' value. Anyway I'm ready to implement the solution you like. Please just describe it |
|
ok, to cover the exact usecase you have and still keep it correct, can we have a new attribute |
4c4b52d to
4471938
Compare
4471938 to
32c85d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Jit security check failed to run.
#jit_bypass_commit in this PR to bypass, Jit Admin privileges required.
32c85d7 to
29c86b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ The following Jit checks failed to run:
- software-component-analysis-go
- static-code-analysis-semgrep-pro
#jit_bypass_commit in this PR to bypass, Jit Admin privileges required.
More info in the Jit platform.
Your suggestion solves the problem. I've updated the implementation. Please review it when you have time. |
ndyakov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thank you @pvragov
You're welcome |
Problem: Context cancellation errors are not distinguished in current metrics.
Solution: Introduce a new canceled metric attribute.
Rationale: This is a non-breaking change. Extending the existing status attribute would break backward compatibility.