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

Add LogQL AST walker #3991

Merged
merged 3 commits into from
Jul 21, 2021
Merged

Add LogQL AST walker #3991

merged 3 commits into from
Jul 21, 2021

Conversation

periklis
Copy link
Collaborator

@periklis periklis commented Jul 13, 2021

What this PR does / why we need it:
The following change set provides a simple extension that enables walking the LoqQL AST for parsed and valid LogQL expressions. This helps to build third-party tooling that can inspect and manipulate LogQL queries before hitting the target Loki query-frontend/querier (e.g. prom-label-proxy).

Furthermore this PR changes:

  • The scoping from private to public for all expressions that comprise LogQL to allow type switching when executing the walking function.
  • The MatcherExpr struct includes a simple AppendMatchers method to append new label matchers when executing the walking function.
  • The LogQL optimizer's custom walkSampleExpr is replaced by expr.Walk

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

cc @cyriltovena @owen-d

return e.matchers
}

func (e *matchersExpr) Shardable() bool { return true }
func (e *MatchersExpr) AppendMatchers(m []*labels.Matcher) {
e.matchers = append(e.matchers, m...)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should get the matchers just public. WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I like this suggestion more as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAICS, none of the expression structs exposes any of their field public, e.g. some interesting field would be LiteralExpr.value, *AggrevationExpr.params, OffsetExpr.offset. IMHO ASTs are very sensitive to public scoping of internals. ASTs are often subject of optimizations and thus keeping fields private is a good choice to save your options for later. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Both sound fine to me 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I think ultimately we'll make those public if we move to another package.

Copy link
Collaborator Author

@periklis periklis Jul 20, 2021

Choose a reason for hiding this comment

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

Let's move them to public when we and if we publish this to another package.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Nice change, LGTM.

@cyriltovena cyriltovena merged commit cb337c3 into grafana:main Jul 21, 2021
garrettlish added a commit to garrettlish/loki that referenced this pull request Oct 12, 2021
The follow-up for grafana#3991, it makes using LogQL syntax outside possible, it also makes LogRange implements Expr interface.
cyriltovena pushed a commit that referenced this pull request Oct 19, 2021
* make LogQL syntax scope from private to public

The follow-up for #3991, it makes using LogQL syntax outside possible, it also makes LogRange implements Expr interface.

* make constructor also public

* Revert "make constructor also public"

This reverts commit ab9d0a9.
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

3 participants