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

Loki: Adds an interval paramater to query_range queries allowing a sampling of events to be returned based on the provided interval #1965

Merged
merged 6 commits into from
Apr 27, 2020

Conversation

slim-bean
Copy link
Collaborator

@slim-bean slim-bean commented Apr 20, 2020

What this PR does / why we need it:

This is a possible solution #1779 which creates a new query parameter for range_queries called interval. It is being marked experimental in the docs as it's very possible we will remove this flag one day in favor of a LogQL based solution.

Signed-off-by: Ed Welch [email protected]

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

Sorry, something went wrong.

@@ -229,7 +229,8 @@ func newQuery(instant bool, cmd *kingpin.CmdClause) *query.Query {
cmd.Flag("since", "Lookback window.").Default("1h").DurationVar(&since)
cmd.Flag("from", "Start looking for logs at this absolute time (inclusive)").StringVar(&from)
cmd.Flag("to", "Stop looking for logs at this absolute time (exclusive)").StringVar(&to)
cmd.Flag("step", "Query resolution step width").DurationVar(&q.Step)
cmd.Flag("step", "Query resolution step width, for metric queries. Evaluate the query at the specified step over the time range.").DurationVar(&q.Step)
cmd.Flag("interval", "Query interval, for log queries. Return entries at the specified interval, ignoring those between.").DurationVar(&q.Interval)
Copy link
Member

Choose a reason for hiding this comment

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

I don't want to bikeshed here but I'm worried the difference between step and interval isn't totally clear and might be hard to remember. Would log_step or entry_step be easier here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be more confusing to me personally.

I would prefer to work on making the description clearer if we can do so.

Copy link
Member

Choose a reason for hiding this comment

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

I could be wrong, but I'm concerned about having naming conventions that requires you to pull up the docs to differentiate between the two. I'm okay with this as-is, but I think it's something to think about.

@codecov-io
Copy link

Codecov Report

Merging #1965 into master will decrease coverage by 0.03%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1965      +/-   ##
==========================================
- Coverage   64.35%   64.31%   -0.04%     
==========================================
  Files         129      129              
  Lines        9950     9977      +27     
==========================================
+ Hits         6403     6417      +14     
- Misses       3065     3072       +7     
- Partials      482      488       +6     
Impacted Files Coverage Δ
pkg/logcli/query/query.go 0.00% <0.00%> (ø)
pkg/logql/evaluator.go 90.69% <ø> (ø)
pkg/querier/http.go 10.19% <0.00%> (ø)
pkg/loghttp/query.go 40.70% <20.00%> (-0.96%) ⬇️
pkg/loghttp/params.go 91.30% <83.33%> (-1.20%) ⬇️
pkg/logql/engine.go 90.64% <100.00%> (+0.58%) ⬆️
pkg/promtail/targets/tailer.go 73.86% <0.00%> (-4.55%) ⬇️
pkg/promtail/targets/filetarget.go 68.29% <0.00%> (-1.83%) ⬇️

Comment on lines 85 to 96
if d, err := strconv.ParseFloat(value, 64); err == nil {
ts := d * float64(time.Second)
if ts > float64(math.MaxInt64) || ts < float64(math.MinInt64) {
return 0, errors.Errorf("cannot parse %q to a valid duration. It overflows int64", value)
}
return time.Duration(ts), nil
}
// Or parse as a duration
if d, err := model.ParseDuration(value); err == nil {
return time.Duration(d), nil
}
return 0, errors.Errorf("cannot parse %q to a valid duration", value)
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole thing could be a function, parseDuration and reused in step and interval.

for ; respSize < size && i.Next(); respSize++ {
// lastEntry should be a really old time so that the first comparison is always true, we use a negative
// value here because many unit tests start at time.Unix(0,0)
lastEntry := time.Unix(-100, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be an external const.

var minTime =...

(i.Entry().Timestamp.Equal(lastEntry.Add(interval)) || i.Entry().Timestamp.After(lastEntry.Add(interval)))
backwardShouldOutput := dir == logproto.BACKWARD &&
(i.Entry().Timestamp.Equal(lastEntry.Add(-interval)) || i.Entry().Timestamp.Before(lastEntry.Add(-interval)))
// If step == 0 output every line.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If step == 0 output every line.
// If interval == 0 output every line.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

Let's make this experimental, I'd like to explore a way to have this in the logql language itself but that is may be too far away so I think we should merge this.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…ng of events to be returned based on the provided interval

Signed-off-by: Ed Welch <[email protected]>
Signed-off-by: Ed Welch <[email protected]>
@slim-bean slim-bean merged commit c7a1a1f into master Apr 27, 2020
@slim-bean slim-bean deleted the interval branch April 27, 2020 14:09
cyriltovena added a commit to cyriltovena/loki that referenced this pull request Apr 29, 2020
cyriltovena added a commit that referenced this pull request Apr 29, 2020
* Fix a bad rebase between #1970 and #1965.

Signed-off-by: Cyril Tovena <[email protected]>

* fmt

Signed-off-by: Cyril Tovena <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants