-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
http fault: implement header controlled faults #6318
Conversation
Part of #5942 Signed-off-by: Matt Klein <mklein@lyft.com>
@rshriram do you mind taking a first pass? |
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
header_delay: {} | ||
percentage: | ||
numerator: 100 | ||
response_rate_limit: |
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.
per previous discussion, i suggest cutting the percentage here and opting for a semantic like, "if you specify delay, it will be injected with 100% certainty". That would make it easier for people to test, imo
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.
See above 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.
mainly comments on doc
Signed-off-by: Matt Klein <mklein@lyft.com>
@rshriram updated |
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
Signed-off-by: Matt Klein <mklein@lyft.com>
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 with a few minor nits
Signed-off-by: Matt Klein <mklein@lyft.com>
@alyssawilk updated |
@alyssawilk updated, lmk if that is what you had in mind. |
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.
Fantastic - that's what I was hoping for!
Part of #5942
Risk Level: Low, does refactor some shared logic between filters, so not entirely
new feature only.
Testing: UTs and integration tests added.
Docs Changes: Added.
Release Notes: Added.