-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[TSVB] Add Positive Rate to Aggregations #59843
[TSVB] Add Positive Rate to Aggregations #59843
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
6ffb764
to
0ae5497
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
expected head sha didn’t match current head ref. |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Just FYI: We still have that on our review list, thanks a lot for creating this PR, we're just working through the more urgent bug fixes right now for the upcoming release, before we'll get to this. |
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 one minor comment (about translation ids).
Tested in Chrome and couldn't find a situation where rate
would return something else than positive only of derivative of max of the same field (tested all visualizations, grouping by various things, overriding index pattern, adding filters, with rollup index).
src/legacy/core_plugins/vis_type_timeseries/public/components/aggs/rate.js
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
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.
Code LGTM.
Tested locally, everything seems to work in the exact way as with using the max-derivative-positive-only method. Saving/reloading works fine.
I've just one small doubt: is it allowed to use unit multiples like 2s
5m
3w
?
@markov00 Yes, you can put anything in there that is supported by the underlying Keep in mind that |
@wylieconlon The goal is to create an aggregation that's a shortcut for the Another approach would be to add "positive only" flag as an option for @sorantis Thoughts? |
@simianhacker Can you explain why this is the standard? I understand that you're trying to take a best practice that works for your specific use case, but I'm trying to think about how this applies to any dataset. |
@wylieconlon With regards to metrics in general, we (Elastic Stack) kind of have a bigger problem because we don't have an reliable way to distinguish the field (metric) type, either gauge or counter. This makes it difficult to guide our users to the right aggregations. If they choose a field that's a counter and then ask for a rate, we should automatically apply For gauge types, "rate" takes on a different meaning because at that point, the question is "Is the value increasing or decreasing?" With counters, that question isn't really important because it's always increasing. BUT we can still ask that question once the counter is converted to a rate, you just need to take a second derivative. Context matters when it comes to "rates". The problem is we have no idea which context we are dealing with, we are stuck trying to come up with names that might guide the user in the right direction. In the "gauge" context, I think this is mostly a naming issue and we need to come up with something that describes the result. I'm starting to think maybe "Growth Rate" is a better name since that's really what we are doing. Side note: We also have an issue with metrics like |
Discussed a bit offline, and I think we clarified this a lot. The new function is not meant to be used for all rates, the derivative function is still useful for those. This function should only be used for numbers that are monotonically increasing, such as disk IO. The "positive only" part of this aggregation is used when the increasing number resets to 0, such as when it hits the max integer. Based on this, I think we need to change the name and add a line of help text explaining what problem this is solving. |
Adding to Marko's question here with time units. While you can do that technically, is there any reasonable use-case why a user would want to use something except I currently don't see any use-cases where users would want to have it on an "non 1" unit. If there are some I'd interested in hearing them, also if we keep the input box, we def want to add validation on it using the |
@timroes I changed "units" to "scale" and changed the text field to a select box. If the user needs a custom interval, they can always use "the advance way" ( |
After some offline discussions, I've decided to rename this to "positive rate" since "growth rate" is a little misleading; growth rates are usually presented as percents. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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 name and behavior are working as expected! I did not review the code of this.
* [TSVB] Add Rate to Aggregations * Fixing i18n labels * Changing from rate to growth_rate; adding message to aggregation form; * Change units to scale; change free text to combobox * Fixing placeholder * Fixing i18n label * Changing from Growth Rate to Positive Rate Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
* [TSVB] Add Rate to Aggregations * Fixing i18n labels * Changing from rate to growth_rate; adding message to aggregation form; * Change units to scale; change free text to combobox * Fixing placeholder * Fixing i18n label * Changing from Growth Rate to Positive Rate Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
This PR add
positive_rate
as a new aggregation to TSVB. This is essentially a shortcut for creating rates in TSVB. Instead of the user having to selectmax
of a field, thenderivative
of the max, thenpositive_only
they can now just selectrate
and choose the field.Before
After
Checklist
Delete any items that are not applicable to this PR.