[TSVB] Add Positive Rate to Aggregations - #59843

Pinging @elastic/kibana-app (Team:KibanaApp)

@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.

@elasticmachine merge upstream

@wylieconlon The goal is to create an aggregation that's a shortcut for the max-derivative-positive-only combination that has become the standard in the "metrics" use case. We've seen a lot of people (externally/internally) use derivative incorrectly and we are trying to guide them towards something more sensible. Maybe Positive Only Rate would be a better name or your suggestion Max Positive Change.

Another approach would be to add "positive only" flag as an option for rate, enabled by default.

@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.

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 max first thenderivate (and most likely positive_only too since counters have to reset at the upper bounds of their type). At that point, the intent of the user is to create a histogram of the positive changes over time; counters grow at different rates.

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, rate in this PR is usually going to be wrong; for the "counter" context it's right. Most of the time, users are confronted with this when they are working with Metricbeat data like system.network.out.bytes. But as we expand into more general metrics use cases, this will keep coming up.

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 system.network.out.bytes because when they are reported there are other important attributes like system.network.interface that must be considered when aggregating. If you want to know the "total" outbound traffic on a host, then you need to calculate the rate for ALL the interfaces (individually) and then add them together. Unless you filter the metric down to system.network.interface, using max on system.network.out.bytes will give you the rate for the interface who's seen the most traffic. Systems like Prometheus avoid these types of pitfalls because the default behavior is to split by ALL attributes.

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 1 as a unit for those? I am thinking in terms of replacing that freeform text input (which otherwise would need some validation that you can't input invalid ES intervals), into a selectbox that just allows you to select "per second", "per minute", "per hour", etc.

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 isValidEsInterval utility from the data plugin.

@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" (max, derivative, positive_only) and scale it via the derivative aggregation. Down the road, if we get enough push back from users that they want that field to be "free form", we can always switch it back since the values are going to be the same.

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 Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 953509f

History

• 💚 Build #38949 succeeded e2a0979
• 💔 Build #38904 failed 1469436
• 💚 Build #38883 succeeded 08a82db
• 💚 Build #38585 succeeded 17ca5f3
• 💚 Build #38292 succeeded e1cea8e

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

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.