Implement offset modifier for range vector aggregation in LogQL #3455

garrettlish · 2021-03-09T10:24:25Z

What this PR does / why we need it:
Enable offset modifier for range vector aggregation in LogQL. The offset modifier allows changing the time offset for range vectors in a query to support e.g. selective timeshift in Grafana.

Which issue(s) this PR fixes:
Fixes #2785

Special notes for your reviewer:

Checklist

Documentation added
Tests updated

CLAassistant · 2021-03-09T10:24:30Z

All committers have signed the CLA.

implementation for grafana#2785, the offset modifier allows changing the time offset for range vectors in a query to support e.g. selective timeshift in Grafana Signed-off-by: garrettlish <garrett.li.sh@gmail.com>

garrettlish · 2021-03-10T09:24:39Z

cc @wardbekker FYI

garrettlish · 2021-03-10T09:42:59Z

the major use case for offset modifier is comparing current data with historical data.

For example (generate an alert if log count increased 50% compares to 3 hours ago):

count_over_time({job="varlogs"} [5m])/count_over_time({job="varlogs"} [5m] offset 3h) > 1.5

garrettlish · 2021-03-11T04:48:27Z

@owen-d WDYT?

owen-d · 2021-03-18T11:50:05Z

Looks interesting! I'll find some time to give this a proper review.

owen-d

The PR looks great - I'm always impressed when people dive into the logql code. Nice work!

I'd like a test or two in engine_test.go that verifies we're only querying data at the correct bounds (code looks good, but would like tests to mirror that). Other than that and some small suggestions, it looks good.

owen-d · 2021-03-19T12:50:18Z

pkg/logql/ast.go

@@ -499,6 +499,7 @@ func newUnwrapExpr(id string, operation string) *unwrapExpr {
 type logRange struct {
 	left     LogSelectorExpr
 	interval time.Duration
+	offset   *offsetExpr


small suggestion: it might be easier to simply have offset as time.Duration. since offset=0 is the same as a nil offset, we could easily do

start := q.Start().Add(-rangExpr.left.interval).Add(-rangExpr.left.offset.offset)

rather than using the more clunky if statements:

if rangExpr.left.offset != nil { start := start.Add(-rangExpr.left.offset.offset) end = end.Add(-rangExpr.left.offset.offset) }

good idea, will make the change

owen-d · 2021-03-19T13:03:45Z

pkg/logql/expr.y

@@ -133,19 +135,31 @@ logExpr:
    ;

 logRangeExpr:


I don't love how we're duplicating all paths here. I know we've done this in the past, but it's getting out of hand (not your fault).

@cyriltovena we should consider refactoring this a bit, but I'm fine with this PR.

owen-d · 2021-03-19T13:17:54Z

pkg/logql/evaluator.go

 	iter := newRangeVectorIterator(
 		it,
 		expr.left.interval.Nanoseconds(),
 		q.Step().Nanoseconds(),
-		q.Start().UnixNano(), q.End().UnixNano(),
+		q.Start().UnixNano(), q.End().UnixNano(), offset,


I think we can simplify this as well. There's no need to make the iterator offset aware if we just pass (start-offset, end-offset) to it.

@owen-d thanks for your comments. I am passing the offset to iterator is because I'd like to shift the returned samples timestamp from history timestamp to current timestamp, otherwise, the returned samples timestamp are historical timestamp which is not meaningful. Any thoughts? Please advise, thanks!

For example:
Assume current time is 17:00, expression - count_over_time({job="varlogs"} [1m] offset 3h) returns 14:00 samples, but count_over_time({job="varlogs"} [1m]) returns 17:00 samples, then the expression - count_over_time({job="varlogs"} [1m])/count_over_time({job="varlogs"} [1m] offset 3h) > 1.5 cannot work since these sub-expressions are returning inconsistent timestamp.

I originally had the same assumption as Owen, eg. you could just pass in the modified start/end.

But now with your explanation I understand why it's like this.

I think you could only offset back L148 only and that would be fine.

loki/pkg/logql/range_vector.go

Line 148 in 05e14f9

ts := r.current / 1e+6

thanks @cyriltovena for the correction, I noticed the same thing and made changed in https://github.com/grafana/loki/pull/3455/files#diff-f4c7f355512099579b5ea8fa4ceceb6d1eeb2b99b10233b22e0e12ce571b25d1R152 already

I think you should add a test with an offset different than zero for the range vector iterator.

Otherwise LGTM for me.

make sense, let me add test, thanks @cyriltovena

@cyriltovena added non-zero offset test - 1b8f454, please help review again, thanks!

owen-d · 2021-03-19T13:18:21Z

pkg/logql/range_vector.go

 }

 func newRangeVectorIterator(
 	it iter.PeekingSampleIterator,
-	selRange, step, start, end int64) *rangeVectorIterator {
+	selRange, step, start, end, offset int64) *rangeVectorIterator {


Mirroring previous comment, I don't think we need to make the iterator offset aware.

garrettlish · 2021-03-22T08:09:39Z

@owen-d thanks for your comments, I had resolved your comments except making the iterator offset aware (since I'd like to shift the returned samples timestamp from history timestamp to current timestamp), please help review the PR again, thanks!

cyriltovena

LGTM

cyriltovena · 2021-03-22T10:22:44Z

Thanks @garrettlish for your contribution, awesome work !

Bring back offset modifier doc section from grafana#3455 and lost in grafana#4012

Bring back offset modifier doc section from #3455 and lost in #4012 **What this PR does / why we need it**: **Which issue(s) this PR fixes**: Fixes regression from #2785 **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [ ] Tests updated **N/A** - [ ] `CHANGELOG.md` updated **N/A** - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label **N/A** - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` **N/A** - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) **N/A**

Bring back offset modifier doc section from #3455 and lost in #4012 **What this PR does / why we need it**: **Which issue(s) this PR fixes**: Fixes regression from #2785 **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [ ] Tests updated **N/A** - [ ] `CHANGELOG.md` updated **N/A** - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label **N/A** - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` **N/A** - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) **N/A** (cherry picked from commit 8645519)

Backport 8645519 from #10960 --- Bring back offset modifier doc section from #3455 and lost in #4012 **What this PR does / why we need it**: **Which issue(s) this PR fixes**: Fixes regression from #2785 **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [ ] Tests updated **N/A** - [ ] `CHANGELOG.md` updated **N/A** - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label **N/A** - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` **N/A** - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) **N/A** Co-authored-by: Kevin Burek <kb000@users.noreply.github.com>

Bring back offset modifier doc section from grafana#3455 and lost in grafana#4012 **What this PR does / why we need it**: **Which issue(s) this PR fixes**: Fixes regression from grafana#2785 **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [ ] Tests updated **N/A** - [ ] `CHANGELOG.md` updated **N/A** - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label **N/A** - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` **N/A** - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) **N/A**

pull-request-size bot added the size/XXL label Mar 9, 2021

Implement offset modifier for range vector aggregation in LogQL

57dab90

implementation for grafana#2785, the offset modifier allows changing the time offset for range vectors in a query to support e.g. selective timeshift in Grafana Signed-off-by: garrettlish <garrett.li.sh@gmail.com>

garrettlish force-pushed the offset branch from 71e9a35 to 57dab90 Compare March 9, 2021 10:45

garrettlish added 2 commits March 10, 2021 18:07

add docs for offset modifier

82a7e84

refine offset expr

05e14f9

garrettlish mentioned this pull request Mar 18, 2021

Implement PromQL offset modifier for range vector in LogQL #2785

Closed

owen-d self-requested a review March 18, 2021 11:49

owen-d reviewed Mar 19, 2021

View reviewed changes

owen-d added advice Issue that illustrates a problem and possible resolution. and removed advice Issue that illustrates a problem and possible resolution. labels Mar 21, 2021

add offset unit test in engine.go

d6e39f9

garrettlish added 2 commits March 22, 2021 16:56

remove duplicated offset shift

eb53fde

add offset test in range vector test

1b8f454

cyriltovena approved these changes Mar 22, 2021

View reviewed changes

cyriltovena merged commit f70c7ea into grafana:master Mar 22, 2021

james-callahan mentioned this pull request Dec 10, 2021

Document offset modifier #4908

Closed

kb000 added a commit to kb000/loki that referenced this pull request Oct 18, 2023

Document offset modifier

85e1115

Bring back offset modifier doc section from grafana#3455 and lost in grafana#4012

kb000 mentioned this pull request Oct 18, 2023

Docs: LogQL offset modifier #10960

Merged

7 tasks

grafanabot mentioned this pull request Oct 19, 2023

[release-2.9.x] Docs: LogQL offset modifier #10973

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement offset modifier for range vector aggregation in LogQL #3455

Implement offset modifier for range vector aggregation in LogQL #3455

garrettlish commented Mar 9, 2021 •

edited

Loading

CLAassistant commented Mar 9, 2021 •

edited

Loading

garrettlish commented Mar 10, 2021

garrettlish commented Mar 10, 2021

garrettlish commented Mar 11, 2021

owen-d commented Mar 18, 2021

owen-d left a comment •

edited

Loading

owen-d Mar 19, 2021

garrettlish Mar 22, 2021

owen-d Mar 19, 2021 •

edited

Loading

owen-d Mar 19, 2021

garrettlish Mar 22, 2021 •

edited

Loading

cyriltovena Mar 22, 2021

garrettlish Mar 22, 2021

cyriltovena Mar 22, 2021

cyriltovena Mar 22, 2021

garrettlish Mar 22, 2021

garrettlish Mar 22, 2021

owen-d Mar 19, 2021

garrettlish commented Mar 22, 2021

cyriltovena left a comment

cyriltovena commented Mar 22, 2021

Implement offset modifier for range vector aggregation in LogQL #3455

Implement offset modifier for range vector aggregation in LogQL #3455

Conversation

garrettlish commented Mar 9, 2021 • edited Loading

CLAassistant commented Mar 9, 2021 • edited Loading

garrettlish commented Mar 10, 2021

garrettlish commented Mar 10, 2021

garrettlish commented Mar 11, 2021

owen-d commented Mar 18, 2021

owen-d left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owen-d Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrettlish Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrettlish commented Mar 22, 2021

cyriltovena left a comment

Choose a reason for hiding this comment

cyriltovena commented Mar 22, 2021

garrettlish commented Mar 9, 2021 •

edited

Loading

CLAassistant commented Mar 9, 2021 •

edited

Loading

owen-d left a comment •

edited

Loading

owen-d Mar 19, 2021 •

edited

Loading

garrettlish Mar 22, 2021 •

edited

Loading