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

Literal Expressions in LogQL #1677

Merged
merged 26 commits into from
Feb 14, 2020
Merged

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented Feb 11, 2020

What

Introduces numeric literals!

Description

Binary arithmetic operators are now defined between two literals (scalars), a literal and a vector, and two vectors.

Between two literals, the behavior is obvious: they evaluate to another literal that is the result of the operator applied to both scalar operands (1 + 1 = 2).

Between a vector and a literal, the operator is applied to the value of every data sample in the vector. E.g. if a time series vector is multiplied by 2, the result is another vector in which every sample value of the original vector is multiplied by 2.

Between two vectors, a binary arithmetic operator is applied to each entry in the left-hand side vector and its matching element in the right-hand vector. The result is propagated into the result vector with the grouping labels becoming the output label set. Entries for which no matching entry in the right-hand vector can be found are not part of the result.

Examples

Implement a health check with a simple query:

1 + 1

Double the rate of a a log stream's entries:

sum(rate({app="foo"})) * 2

@codecov-io
Copy link

codecov-io commented Feb 11, 2020

Codecov Report

Merging #1677 into master will increase coverage by 0.61%.
The diff coverage is 73.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1677      +/-   ##
==========================================
+ Coverage   62.81%   63.43%   +0.61%     
==========================================
  Files         112      121       +9     
  Lines        8741     9011     +270     
==========================================
+ Hits         5491     5716     +225     
- Misses       2837     2877      +40     
- Partials      413      418       +5
Impacted Files Coverage Δ
pkg/promtail/targets/journaltargetmanager_linux.go 0% <ø> (ø) ⬆️
pkg/promtail/targets/filetargetmanager.go 0% <ø> (ø) ⬆️
pkg/promtail/targets/manager.go 0% <ø> (ø) ⬆️
pkg/promtail/targets/journaltarget.go 53.73% <ø> (ø) ⬆️
pkg/querier/queryrange/roundtrip.go 80.43% <0%> (ø) ⬆️
pkg/logql/ast.go 88.18% <0%> (+0.19%) ⬆️
pkg/logcli/query/query.go 0% <0%> (ø) ⬆️
pkg/querier/queryrange/split_by_interval.go 87.23% <0%> (ø) ⬆️
pkg/loghttp/query.go 45% <0%> (-2.37%) ⬇️
pkg/logql/engine.go 91.19% <100%> (+3.31%) ⬆️
... and 31 more

@owen-d
Copy link
Member Author

owen-d commented Feb 12, 2020

Don't merge this until I add support for a response result type for Scalar. We currently only have

// ResultType values
const (
	ResultTypeStream = "streams"
	ResultTypeVector = "vector"
	ResultTypeMatrix = "matrix"
)

and I should add another

pkg/logql/ast.go Outdated Show resolved Hide resolved
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.

Great work !

@owen-d
Copy link
Member Author

owen-d commented Feb 13, 2020

Hey @cyriltovena I updated this to properly use Scalar response types and could use another review.

The ugly:

  • There's a lot of new unsafe code in the engine/evaluators. Please let me know if you have better ideas for how to impl this.
  • I'm confused about the marshaling/unmarshaling paths and it doesn't seem we have any guarantees of correctness when refactoring. I'd love to be able to prove that adding a LogQL result type will automatically work with the queriers & frontend. Currently there are so many layers of indirection (for cortex compatibility, etc) that it's hard to follow.

I'll integration test this tomorrow or Monday.

[edit]

I found a way to improve the evaluator code. This hinges on proving that a binary expression necessarily produces a vector evaluator. This is assured because all binary ops where both legs are literals can be recursively reduced at construction ( (1+1) + 1) -> 3. This happens here:
https://github.com/grafana/loki/pull/1677/files#diff-bdc855598cd74ac92fb413d7a38b4f53R407-R409

Once that's assured, we simply check if the root node of an AST is a literal and handle that edge case. Otherwise, all subsequent evaluators will produce vectors which allows us to reduce the type complexity therein.

@@ -288,6 +288,21 @@ func TestEngine_NewInstantQuery(t *testing.T) {
promql.Sample{Point: promql.Point{T: 60 * 1000, V: 0.2}, Metric: labels.Labels{labels.Label{Name: "app", Value: "fuzz"}}},
},
},
{
// healthcheck
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a test for Literal + Vector ?

{`topk(3,count_over_time({foo="bar"}[5m])) by (foo,bar)`, []int{TOPK, OPEN_PARENTHESIS, IDENTIFIER, COMMA, COUNT_OVER_TIME, OPEN_PARENTHESIS, OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, DURATION, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS, BY, OPEN_PARENTHESIS, IDENTIFIER, COMMA, IDENTIFIER, CLOSE_PARENTHESIS}},
{`bottomk(10,sum(count_over_time({foo="bar"}[5m])) by (foo,bar))`, []int{BOTTOMK, OPEN_PARENTHESIS, IDENTIFIER, COMMA, SUM, OPEN_PARENTHESIS, COUNT_OVER_TIME, OPEN_PARENTHESIS, OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, DURATION, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS, BY, OPEN_PARENTHESIS, IDENTIFIER, COMMA, IDENTIFIER, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS}},
{`topk(3,count_over_time({foo="bar"}[5m])) by (foo,bar)`, []int{TOPK, OPEN_PARENTHESIS, NUMBER, COMMA, COUNT_OVER_TIME, OPEN_PARENTHESIS, OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, DURATION, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS, BY, OPEN_PARENTHESIS, IDENTIFIER, COMMA, IDENTIFIER, CLOSE_PARENTHESIS}},
{`bottomk(10,sum(count_over_time({foo="bar"}[5m])) by (foo,bar))`, []int{BOTTOMK, OPEN_PARENTHESIS, NUMBER, COMMA, SUM, OPEN_PARENTHESIS, COUNT_OVER_TIME, OPEN_PARENTHESIS, OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, DURATION, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS, BY, OPEN_PARENTHESIS, IDENTIFIER, COMMA, IDENTIFIER, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS}},
{`sum(max(rate({foo="bar"}[5m])) by (foo,bar)) by (foo)`, []int{SUM, OPEN_PARENTHESIS, MAX, OPEN_PARENTHESIS, RATE, OPEN_PARENTHESIS, OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, DURATION, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS, BY, OPEN_PARENTHESIS, IDENTIFIER, COMMA, IDENTIFIER, CLOSE_PARENTHESIS, CLOSE_PARENTHESIS, BY, OPEN_PARENTHESIS, IDENTIFIER, CLOSE_PARENTHESIS}},
Copy link
Contributor

@cyriltovena cyriltovena Feb 14, 2020

Choose a reason for hiding this comment

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

Would be nice to have some new tests here too.

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.

LGTM

@owen-d owen-d merged commit 414f95f into grafana:master Feb 14, 2020
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.

3 participants