Skip to content
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

[Lens] Use rate agg behind the scenes for counter rates on mode time_series data views #130714

Closed
Tracked by #153629
ghudgins opened this issue Apr 20, 2022 · 9 comments
Closed
Tracked by #153629
Assignees
Labels
enhancement New value added to drive a business result Feature:Lens impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@ghudgins
Copy link

ghudgins commented Apr 20, 2022

Describe the feature: With new support for restart-aware rates within Elasticsearch itself (and ECS) Lens should automatically switch rate implementations to the underlying Elasticsearch approach.

  • Default aggregation: sum (for "total rates")
  • Fallback for mixed mode data views: In the event a field exists in both mode: time_series and normal indexes then fall back onto the browser implementation that exists today for rates. Consider displaying a warning in the editor when this happens: "Data selection includes non-time series indexes and rates are calculated in the browser." (Seems like something that isn't actionable though...discuss)
  • Formula implication: Are there any changes needed?

Example aggregation call for pure time series data views

date_histogram {
  time_series {
   rate my.counter.metric {}
  }
  sum_bucket time_series > rate {}
}

No UX impact:
image

Describe a specific use case for the feature:
When My machines collecting counters periodically restart
I need restart-aware rates at the aggregation level
So I can avoid distorted visualizations from today's browser implementation

@ghudgins ghudgins added enhancement New value added to drive a business result Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Apr 20, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@ghudgins ghudgins changed the title [Lens] Use rate agg for TSDB counter metrics [Lens] Use rate agg behind the scenes for counter rates on mode time_series data views Apr 20, 2022
@flash1293
Copy link
Contributor

Formula implication: Are there any changes needed

Yes, because right now a counter rate looks like this in formula: counter_rate(max(field)). I think we can migrate it in an elegant way though:

  • Turn a counter rate into counter_rate(field) instead (this way Lens can switch the underlying implementation from server side rate to client side rates easily without the user noticing)
  • Add an argument for how time series should be aggregated: counter_rate(field, aggregate=sum)
  • Migrate existing formulae to the new approach as long as the argument is strictly counter_rate(max(field)), otherwise replace with the equivalent clamp(0, differences(< old formula >), differences(< old formula >)) - I expect this case to be super rare

@flash1293
Copy link
Contributor

Fallback for mixed mode data views

There are two scenarios in here - if the data view was mixed from the start, IMHO Lens simply shouldn't offer the option to configure the "aggregate series" and behave like it would for regular data views. If the data view was fully TSDB in the past and the user already configured the "aggregate series", then keep it and try to execute it - it will work for the TSDB data and return no data for the regular indices which is IMHO a sensible behavior.

The only issue in this regard is the way integrations within metrics-* are structured - often, different integrations populate the same fields (e.g. core system metrics). In a transition phase some of them will be TSDB and others won't be because most likely not all integrations will be able to switch to TSDB at the same time. So the user will not be able to configure the "aggregate series" for these even though it would work in some places.

@ghudgins
Copy link
Author

ghudgins commented Jun 17, 2022

Discussion yielded this desired design for counter rates -

  • assume a counter rate for counter field and have the user pick the quick function that summarizes the rate (average / min / max / sum). (We could not identify any use case for offering non-rate aggregations as they don't result in data that makes sense.)
  • gray out quick functions that don't work with rate agg
  • error message if you pick something else--same as what we do for invalid field types today (picking a string for an average today, for example)

This is a separate issue but is worth creating and discussing: We likely need to deprecate lens' "counter rate" quick function as it exists today.

CC @MichaelMarcialis @flash1293

@ppisljar
Copy link
Member

looking into this, it seems:

  • tsdb counter metric field can not be used in aggregations the way other fields can
  • the only aggregation that is supported is rate aggregation and it MUST be nested under the time_series aggregation

lens must be updated so correct aggregation is built and that no options that would produce invalid request are allowed

@stratoula stratoula added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Jan 11, 2023
@martijnvg
Copy link
Member

Just checking in to see whether there are issues here that we can help with on the Elasticsearch side.

tsdb counter metric field can not be used in aggregations the way other fields can

That is only the case if computing a rate with reset detection is required. Other aggregations remain to work on counter fields, it is just a number field for the aggs. For example a max aggregation just works on a counter field.

the only aggregation that is supported is rate aggregation and it MUST be nested under the time_series aggregation

Yes, the rate aggregation must be wrapped under the time_series aggregation. But only if a rate with reset detection is required.

@timductive
Copy link
Member

Tagging the WIP to this issue to make it easier to see progress:
Elasticsearch PR: elastic/elasticsearch#90447
Kibana PR: #147987

@stratoula
Copy link
Contributor

This is done in 8.8, we have another issue for optimization but TSDB counter fields are now supported in Lens

@drewdaemon
Copy link
Contributor

Just to be clear here, we did not end up using the ES rate agg behind the scenes. Instead, we added support for TSDB while still using Lens's client-side counter_rate calculation.

#152537 is the issue to track for actually using the ES rate agg in Lens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Lens impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants