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

filodb(core) include the start time for range functions such as rate. #1665

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

yu-shipit
Copy link
Contributor

@yu-shipit yu-shipit commented Sep 9, 2023

Pull Request checklist

  • The commit(s) message(s) follows the contribution guidelines ?
  • Tests for the changes have been added (for bug fixes / features) ?
  • Docs have been added / updated (for bug fixes / features) ?

Current behavior : (link exiting issues here : https://help.github.com/articles/basic-writing-and-formatting-syntax/#referencing-issues-and-pull-requests)

New behavior :

The result of a range vector function is based on the data in a range. Previous the first millisecond of data was excluded from the range, now it is included. Let's say, we have a ts counter { [0s, 1.0], [5s, 6], [10s, 16].

Previously, rate(count)[10s] at timestamp 10 just has two data points [5s, 6] and [10s, 16] it may result in a result (16 - 6) /5 =2.

Now, the [0s, 1.0] is included so, the rate would be (16 - 1) / 10 = 1.5

After this change, Filodb will be compliant to Prometheus.
BREAKING CHANGES

Breaking changes may include:

  • Any schema changes to any Cassandra tables
  • The serialized format for Dataset and Column (see .toString methods)
  • Over the wire formats for Akka messages / case classes
  • Changes to the HTTP public API
  • Changes to query parsing / PromQL parsing

Other information:

@yu-shipit yu-shipit force-pushed the range_vector_function_fix branch from 29eb63b to 98e5952 Compare September 9, 2023 06:23
@@ -155,16 +155,17 @@ final case class PeriodicSamplesMapper(startMs: Long,
* pronounced if [1i] notation was used and step == lookback.
*/
private def extendLookback(rv: RangeVector, window: Long): Long = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we include the start time, we do not need to extend the lookback window anymore.
Here is an example, the window is 15s and no need to extend the window to 15001ms
[2023-09-12 15:47:51,816] INFO query-sched-1 default f.query.Query$ [] - functionId=Some(Rate) stepMultipleNotationUsed=false window=15000 rv=RawDataRangeVector(/shard:3/b2[schema=prom-counter _metric_=my_counter,tags={_ns_: Test-data-1694557937_p1, _ws_: aci-telemetry, code: 817, instance: inst-4, mode: nice, service: service-2, version: 3.3.13}] [grp16],TimeSeriesPartition(shard=3,partId=112){b2[schema=prom-counter _metric_=my_counter,tags={_ns_: Test-data-1694557937_p1, _ws_: aci-telemetry, code: 817, instance: inst-4, mode: nice, service: service-2, version: 3.3.13}]},TimeRangeChunkScan(1694557922000,1694557937000),[I@65c327b0,0,200000000,dcd2633e-6b01-4d89-8d73-124b18cacdfd)

@alextheimer
Copy link
Contributor

@yu-shipit Could you include a quick summary of the PR in the header comment?

Copy link
Contributor

@alextheimer alextheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most changes look good-- just a couple questions. Thanks for the tedious work here @yu-shipit 🙏

@yu-shipit yu-shipit requested a review from alextheimer November 8, 2023 18:14
@yu-shipit yu-shipit merged commit fdb74d6 into filodb:develop Nov 16, 2023
1 check passed
@yu-shipit yu-shipit deleted the range_vector_function_fix branch November 16, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants