Prometheus: allow step to be smaller than $__interval

[davkal]

What this PR does / why we need it:

In #14209 there is a request for uncoupling step and interval. This PR treats them separately.

Current behavior:

After this change, you can have a higher resolution, independent of the dynamically calculated interval:

Open questions:

• should the interval factor be applied to both still? I'd say yes, In effect that determines how many datapoints are being returned

Todos:

• Fix tests once behavior is decided

[torkelo]

Not sure about this, could this not have pretty major breaking behavior and also result in A LOT more data being queried for than before? Min step is only setting the minimum.

With this change if minStep is it overrides the automatic data point limiting factors, long time ranges will always hit the prometheus max data limits and return 11K data points per series?

[beorn7]

I'm a bit too tired right now to look through all the implementation details. Especially, I have a really hard time to read the tests. (Where is Min step and Min time interval actually set in the tests?)

In general, I like the idea that the user can independently determine what the min step and the min $__interval is. However, I think there is some confusion here. Until right now, I've thought that $__interval would always be exactly the same as the step in the Prometheus query. I don't quite understand why they could be different at all. Now I see there are two separate settings for their minimum. (I only ever used Min step so far, and I wrote my queries in the assumption that that would set the same lower limit to both $__interval and step (which I saw as the same). Playing with a dashboard in practice still makes me think that's true, possibly because not setting a Min time interval has exactly that effect.)

May I ask why there are the two separate settings? Depending on the reason, I might be able to say if they should be independent. The fact alone that there are two different settings suggests they should be independent, so I do understand the confusion of the OP in #14209 . However, the discussion there looks to me like the independence is only required because users want to set a dynamic duration in the PromQL expression that is larger than the step in the Prometheus API call.

[davkal]

Not sure about this, could this not have pretty major breaking behavior

Step still defaults to the dynamically calculated interval based on range and width. This PR simply allows you to dial up the resolution.

and also result in A LOT more data being queried for than before? Min step is only setting the minimum. With this change if minStep is it overrides the automatic data point limiting factors, long time ranges will always hit the prometheus max data limits and return 11K data points per series?

It's still adjusted automatically to respect the 11,000 points limit.

[davkal]

May I ask why there are the two separate settings?

The min interval is a datasource independent roll-up parameter that's dynamcially calculated based on the timepicker range and the screen size. It's shown below every metrics datasource and it's up to the datasource to make use of it. It is also available in the query variable as $__interval.

I'm guessing the min step was introduced to set a per-query step, but the main issue now is that that per-query step is hard-bound by the "min time interval" at the bottom of the queries, making the per-query step not really per-query.

[davkal]

Any opinions from @roidelapluie or @peergynt?

[beorn7]

I think the current duality of step and $__interval is very confusing to those without the historical context behind it.

Let's take a step back and look at what people really try to accomplish with those settings and variables, in particular what @peergynt was aiming for in #14209.

It starts with dashboarding the classical rate(requests_total[2m]) RPS query. Users want the interval in the range selector to increase for graphs covering longer ranges. For example, if each plot point in the graph is 1h apart, they want the rate expression to cover much more than 2m. Otherwise, only a fraction of the data would actually make it into the graph, and something very noisy at higher resolution would look equally noisy at lower resolution. This is where rate(requests_total[$__interval]) came to the rescue. Two caveats with that:

1. You have to make sure that the interval in the range selector doesn't go too low. You want something like 3x or 4x of the scrape interval. That's where Min time interval comes into the game (leading to the sub-caveat that now your dashboard has to know about the scrape rate configured on the Prometheus server – you might call that a leaky abstraction or a layering violation).
2. What people often have in mind when dynamically increasing the interval in the range selector is “full coverage” of the data. However, Prometheus only considers data points within the range interval for the rate calculation. With the above in action, the intervals will directly border each other, thus the delta between the latest point of one interval and the earliest point in the next interval will not make it into any of the rate evaluations. The effect of this is particularly visible on noisy results with a short $__interval. Even with the above recommended limit of 3x to 4x the scrape interval, 25% to 33% of the counter increases will not make it into the final graph.

Currently, there is no obvious solution with the means of Grafana and/or Prometheus as is. What comes to mind as a solution is to extend the range interval by one scrape interval. However, since you cannot do math on the range interval (neither within PromQL nor within Grafana), there is no straightforward way to implement this for dynamic ranges. (You can easily do it for a graph with a fixed range, of course.)

So what do people do? Here, I believe, we have arrived at exactly the situation described in #14209. @peergynt wanted the Prometheus step to be smaller than the $__interval used in the range selector to see all the little peaks missing in the vanilla graph. However, I think this is not just a problem of minimum step and $__interval because it also applies to larger values for step and __interval`, just that the problem becomes the less relevant the more data points are contained within the range interval.

Thus, I think discussing different minimal values for step and $__interval is a distraction. (It might be relevant for other problems, but not for the root problem of #14209.) Instead, could we have some way of allowing math on the Grafana variables? I don't have a good idea about the syntax, but what we want to express is: “My scrape interval is 10s, so Min time interval should be 30s, and the interval in my range selector should be $__interval plus 10s.” It would be great if we could say just that in Grafana.

A totally different angle to approach the problem is to fix it within Prometheus. Thinking about Prometheus being primarily a data source for dashboarding leads to claims that the rate implementation is “broken“, see the comments in #14209 and issues and PRs linked thereof. I don't really want to go down that rabbit hole here. (I have stayed out of that discussion for a number of good reasons.) But I think it's important to acknowledge that Prometheus isn't primarily about feeding dashboards. The most important duty of Prometheus is to allow alerting based on TSDB queries that are as easy to reason with as possible, and from which you can easily understand and explore why an alert is firing (if need be by easily hopping from Prometheus to other components of a complete observability ecosystem). IMHO that's the reason why Prometheus is resisting feature creep in PromQL and elsewhere that solely helps dashboarding but makes life in other situations more complicated. A more straightforward example than the rate one is the ancient problem of graphing the top N time series, for which Prometheans always said there should be a solution in the graphing layer. Some time later, it was beautifully solved in Grafana. In the same way, it would be nice to solve #14209 in an equally elegant way in Grafana.

Finally, to play devil's advocate, one might as well say that all of this has limited relevance because rate queries directly in the dashboard only work for smaller setups. With many series, you want a recording rule to calculate the rate ahead of time, and once you are doing that, all the thoughts about dynamic range intervals in Grafana are moot. A way out of this are recording rules over relatively short ranges (like 4x the scrape interval), which you can then average over via avg_over_time(my_recording_rule:rate2m[$__interval]), in which case you need neither a Min time interval nor an extension of $__interval in the range query. This is hinted at in the RP blog post linked earlier. And it's arguably a solution to all the problems discussed here without any of the complications and without any requirement for new features.

[roidelapluie]

My opinion is simple: there is something missing somewhere. I have tried ${2*__interval} in some dashboards, which did not work :)

[brian-brazil]

How about setting $__interval to step + min_time_interval?

[torkelo]

Regarding $__interval I am planning on making the calculation for this much more transparent and so that you can see & change the formula for how it is calculated.
#18999

Max data points = Width of the panel in pixels / 2  (or some other value)
Interval = Time Range / Max Data Points 

Not everything with that PR / suggestion is ironed out, needs more UX & details figured out.

The main purpose for that is to be able to set limits on the number of data points queries return in a time range agnostic way.

Regarding wanting step lower than $__interval. What if we added a resolution option like 2/1, that way you would get a step that is half of $__interval. But not sure that solves it as currently, the resolution impacts the $__interval also.

[beorn7]

The better explanations above make a lot of sense. But IMHO they enforce the notion that min step and min interval are (or should be) the same. (You even call it now Min interval/step.)

The problem my little essay above was about is that we want a way to template a duration in PromQL that is one scrape interval longer than $__interval. @roidelapluie said more or less the same in his comment. (Just that I would want something like ${__interval + 15s} rather than a factor.)

Assuming that I'm right and my suggestion solves the problem at hand, is there then a reason left why we have that Min step field at all? What are scenarios where it would be different from Min interval?

[davkal]

The range coverage "issue" seems to be prometheus-specific, and hence could be fixed in the datasource itself (instead of waiting for maxpoints logic), e.g., by adding a scrape interval to the dynamically calculated $__interval.

If we did this magically, would this be confusing? I.e., are there enough users out there, who already understand the coverage issue and have manually adapted the step?
From a configuration standpoint, would it semantically be enough to assume that someone who has configured the scrape interval on the existing field would not mind the $__interval being corrected by it? Or would a separate checkbox "Adjust graph interval by adding scrape interval" be needed?

is there then a reason left why we have that Min step field at all?

The change from "Step" to "Min step" was done here: #8073 which seems to ensure that per-query step parameters make sense for short ranges. But it seems like that issue could have been solved by adding a scrape interval to the dynamically calculated 5s already.
One side-effect that was noted as an advantage was that a higher step leads to less data being transferred, so a min step could enforce this. But if you want this you also can use the resolution factor setting. Come to think of it, the resolution select box could become the primary step tweaker (e.g., "1/2" to double the dynamic interval, and we could introduce new settings "2/1" and higher to divide the dynamic interval as suggested by @torkelo ). With those in place, I also dont see any need for a custom step field other than hard-code a step, which we should probably allow again for "total" control (removing the boundary semantic might lead to unexpected results though).

With all the above, I think we can take these steps:

• close this PR
• add the new resolution factors (and relaxing the lower bound by that factor) could already solve the main use case of controlling the resolution in a dynamically calculated interval.
• add an a datasource option to add the scrape interval to the dynamically calculated resolution interval (step)

Thoughts?

[davkal]

Related: #11451

[beorn7]

[…] who has configured the scrape interval on the existing field […]

We do have an existing field to configure the scrape interval?

That would be my first concert here: That Grafana cannot even know the scrape interval. And even if there were a field (or there is, and I just haven't discovered it yet), there is possibly not only one scrape interval. Every target in Prometheus can have different scrape intervals, and while it's recommended to stick to a common interval as far as possible, there are situation in practice where the scrape intervals differs within an organization.

[…] e.g., by adding a scrape interval to the dynamically calculated $__interval. If we did this magically, would this be confusing?

See above, I don't think Grafana can know the scrape interval. Sometimes not even in principle.

But let's assume we have a scrape interval that is "usually" right, and we can configure it in the panel. Then the following question makes sense:

From a configuration standpoint, would it semantically be enough to assume that someone who has configured the scrape interval on the existing field would not mind the $__interval being corrected by it? Or would a separate checkbox "Adjust graph interval by adding scrape interval" be needed?

The answer is that we would definitely need a separate checkbox. That's mostly because users who do use a recording rule over a shorter interval and then want to do the dynamic range with avg_over_time, as I described in the last paragraph of my long comment above, then you must not add the scrape interval to $__interval. Also, in that case Min interval is fine to be set to the rule evaluation interval (instead of a multiple of the scrape interval).

The latter case (using recording rules with a short range and avg_over_time for graphs with dynamic range) is actually the generally applicable (because you must use recording rules if you have so many metrics that the direct dashboard query becomes too slow). We should definitely not destroy the generally applicable strategy.

My suggestion is:

• Get rid of Min step. It is too confusing because it overlaps in subtle way with Min interval.
• Use Min interval to limit the step parameter in the Prometheus query instead.
• Add a new option _Modify $_interval in PromQL range selectors described below.

The new option would allow to modify $__interval only in the case it is used in a PromQL range selector, and there would be two possible modifications:

1. Add a fixed amount in seconds.
2. Set a minimum value in seconds (after applying (1)).

Default value for both would be 0. The user would set (1) to the scrape interval in case they are not using recording rules and want the perfect coverage as described above. In that case, they would also set (2) to something like 3x or 4x the scrape interval. A user that works with recording rules would leave (1) at 0 and would set (2) to the rule evaluation interval (which might be different from the scrape interval). (2) would not override Min interval, i.e. $__interval is calculated as in all data sources, and then the special settings are applied for PromQL range selectors using $__interval only.

[beorn7]

This would satisfy #11451.

[davkal]

We do have an existing field to configure the scrape interval?

[beorn7]

Of course, in the data source!

Then the Min step parameter makes even less sense, as it now overlaps with both Scrape interval and Min interval in difficult to understand ways.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[davkal]

Not going forward with this. Great discussion though @beorn7 ⭐️

[beorn7]

Are there any plans to do something similar to what I suggested above? I'm pretty happy with the insights gained from this whole discussion, but how to put them into action?