-
Notifications
You must be signed in to change notification settings - Fork 628
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
chore(docs): add documentation on the object structure of canary config. #622
Conversation
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.
Would like @csanden and @nisanharamati to review as well.
### Canary Config Object model (object) | ||
|
||
#### Properties | ||
- `name` **my-app golden signals canary config** (string, required) - Name for canary configuration. |
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.
Could also add the id
since it can now be specified on the config when it is pushed.
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.
Not sure how deep this should go, but: in orca, at least one of id and name (but not both) is required. If both are provided, name is ignored and id is used.
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.
- Add id
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.
@nisanharamati, I dug through the Kayenta code and it doesn't have any of that logic. It only basically toggles generating an id or not, which is what I have documented for now. However we can probably add a note in the description about Orca if you want.
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.
I think it's okay to leave out in here. I'll try and think of where would be a good place to document that behaviour.
#### Properties | ||
- `allowedIncrease` **1.1** (number, optional) - Defaults to 1. The multiplier increase that must be met for the metric to fail. This example makes the metric fail when the metric has increased 10% from the baseline. | ||
- `allowedDecrease` **0.90** (number, optional) - Defaults to 1. The multiplier decrease that must be met for the metric to fail. This example makes the metric fail when the metric has decreased 10% from the baseline. | ||
- `criticalIncrease` **5.0** (number, optional) - Defaults to 1. The multiplier increase that must be met for the metric to be a critical failure and fail the entire analysis with a score of 0. This example make the canary fail critically if there is a 5x increase. |
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.
@csanden could you verify the accuracy of criticalIncrease
and criticalDecrease
and there relationship with critical
metrics.
I came up with this from reading the code, and i'm not a Scala expert.
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.
IIRC when a metric goes over a critical threshold, the entire test is terminated to minimize negative production impact
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.
That is correct. If a metric is marked a critical
then we will check if the user has supplied a criticalIncrease
or criticalDecrease
value. These properties allow the user to control the sensitivity of a "critical" failure. Note that, a critical failure will result in the entire canary to be marked as a failure and receive a score of zero.
…ic Insights Metric Set Query Config.
e110fdf
to
a070603
Compare
Co-Authored-By: nisanharamati <hanisan@gmail.com>
Co-Authored-By: nisanharamati <hanisan@gmail.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 once the url typos are fixed
Re: the criticalIncrease
property I'll defer to @csanden
Co-Authored-By: nisanharamati <hanisan@gmail.com>
Co-Authored-By: Chris Sanden <chris.sanden@gmail.com>
As discussed here: #621 (comment)
This is a PR of my internal documentation of the Object structure of the Canary Config that documents some of the hidden features that the UI doesn't expose.
These probably aren't perfect but are better than nothing.
One thing that I am still confused on that maybe @csanden can add input on is the difference between
critical
andallowed
increase / decrease.