-
Notifications
You must be signed in to change notification settings - Fork 175
Add rate and percentile aggregations to docs for custom threshold rule #3676
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
A documentation preview will be available soon. Request a new doc build by commenting
If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here. |
This pull request does not have a backport label. Could you fix it @dedemorton? 🙏
|
@maryam-saeidi I think I've taken this as far as I can without help from the development team. I looked through all the resources you referenced in the issue, but it's really a lot to slog through. I did add a new page called "Understanding rate aggregations" but I suspect that this should probably be refined (written?) by someone who didn't just learn what a rate aggregation was yesterday. :-) The quality of the content perhaps reflects my ignorance. At any rate, it's a starting point. I welcome your comments and guidance to make it better. Plus I would appreciate your input on a good example to provide on that page. |
Hi @dedemorton, Thanks for adding this page! 🙏🏻 How would you like to collaborate, shall I just write information for the sections that you provided in a Google doc and share it here or do you prefer it differently? I will also get feedback from one of our SREs who worked with this aggregation to see what he thinks would be a nice addition to this document. :) |
@maryam-saeidi I'm totally OK with you pushing your changes directly to my fork to make them part of the PR review. If you prefer to work in google docs, that's OK too. I've added you to my fork as a contributor so you can push changes to the branch there (if you want). |
@dedemorton I had a chat with @jasonrhodes and we think it would be better to put this document under a separate category like aggregations, or a separate item. The current place of this document does not match the rest of the items in the same group (which are different rule types): ![]() So, either we can add another item in the following list or a separate group like aggregations:
|
@maryam-saeidi Oh I totally agree. I've shoehorned it into the wrong place. The problem with this content is that it's an outlier compared to how we cover other types of aggregations. We are very uneven in how we cover aggregations and don't provide comprehensive reference documentation about all the aggregations we support in alerting rules. I don't want to have Aggregations at the top level in the nav right now because that will raise expectations for users who want to learn about other aggregations and will be disappointed if we only cover rate aggregations. (Plus it's not a best practice to have a nav container that contains a single page.) For now, I can move the new page up a level in the TOC and rename it. So we'll have something like:
*I want to avoid calling the new page "Rate aggregations" to keep the wording parallel in the nav and prevent folks from accidentally reading "rate" as a verb initially. In the future, we probably need a reference section somewhere in the alerting docs that covers aggregations in more depth, but that's out-of-scope for this PR. If you think it would be valuable, please open an issue, and we'll add it to our sprint planning. Thanks! |
@dedemorton could we make that something like this:
|
@jasonrhodes I can do that if it would help your team build out more content later, but I think users are going to be misled and potentially annoyed by the missing info (plus the extra clicks). But maybe they feel that way already? There's been some scope creep with this work (originally sized at <1 day based on the original description in the issue, which was edited after we did our sizings). That's OK, but it means that the container topic for the new section is going to be very basic. Does that sound OK? |
Of course, whatever makes sense on the scope side of things. I also defer to your expertise on the navigation choices. My perspective is that the "Information Architecture" being communicated by having "Learn about rate aggregations" by itself in the nav would, for me, be more confusing and frustrating than having some of the aggregations missing, but I understand what the page is in part because the IA makes sense to my brain (compared to that drop down that I use when I create/edit rules, in other words).
Update: with the new table structure DeDe added, I don't think we need a new issue for more pages of aggregation explanations. I'll not make that issue, and we can add them if they naturally arise. Thanks for your help, @dedemorton! |
I'll add a commit so you can see how it looks, and we can go from there. |
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.
Thank you @dedemorton for all of the help and back and forth on this, it's looking great imo <3
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.
Pending the last question for @maryam-saeidi, this lgtm!
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! I've added one comment for Rate aggregations
.
Thanks for all your effort to improve our documentation ❤️
6530b77
Adds changes requested in https://github.com/elastic/observability-docs/issues/3530.
TODO:
Then point to the page from both rules.