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: Implement step editor #69648

Merged
merged 16 commits into from Jun 16, 2023
Merged

Loki: Implement step editor #69648

merged 16 commits into from Jun 16, 2023

Conversation

ivanahuckova
Copy link
Member

What is this feature?

In this PR we are adding step editor to Loki query editor. With a new step editor, users can set the value for step. It accepts durations (e.g. 1s, 1d, 1h), numbers and $__interval or $__range variables.

If the user set up the step, we use it. We don't do safeStep evaluation on it, because we want step input to be direct and we don't want "magic where we would adjust step" happening on the backend.

I was thinking about adding validation, but I would like to add interpolation of template variables in the next PRs and then validation becomes more tricky, so I decided not to add it (at the moment). Moreover, other step editors (e.g. in Prometheus), also don't have validation, so I don't consider it as MVP and it can be added later (I added it as task in #58337)

Why do we need this feature?

Users would like to set the value of step parameter for metric query.

Who is this feature for?

Loki users who run metric queries

Which issue(s) does this PR fix?:

Part of #58337

Special notes for your reviewer:

step.mov

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Backend code coverage report for PR #69648

Plugin Main PR Difference
loki 57.9% 57.9% 0%

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Frontend code coverage report for PR #69648

Plugin Main PR Difference
loki 85.26% 85.26% 0%

@matyax
Copy link
Contributor

matyax commented Jun 7, 2023

Not sure if I'm doing anything wrong, but it doesn't matter what I put in step, I always get the same values:

imagen

Edit: I think there's something wrong with my local instance.

@ivanahuckova
Copy link
Member Author

Not sure if I'm doing anything wrong, but it doesn't matter what I put in step, I always get the same values:

hmm I didn't tested with split duration + split queries, so I'll have a look at that. Does it happen if you have split toggles off?

@matyax
Copy link
Contributor

matyax commented Jun 7, 2023

No, it was my local Grafana instance. There was a zombie process with another branch in the background.

imagen

Copy link
Contributor

@matyax matyax left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left a couple of suggestions.

JStickler added a commit to grafana/loki that referenced this pull request Jun 7, 2023
**What this PR does / why we need it**:

In grafana/grafana#69648 we are in Grafana
introducing a step editor in Loki. Unfortunately, the error message when
user sets too low step parameter is hard to understand, so I am
proposing following change to make it more understandable and
actionable.

Let me know what do you think.

---------

Co-authored-by: J Stickler <julie.stickler@grafana.com>
@ivanahuckova
Copy link
Member Author

@gabor @matyax I have implemented using of correct step for query splitting in 32c004d where I split queries based on calculated step, rather than resolution. Could you please have a look.

@ivanahuckova ivanahuckova requested a review from matyax June 9, 2023 13:26
Copy link
Contributor

@matyax matyax left a comment

Choose a reason for hiding this comment

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

Query splitting changes look great! Left a comment.

@ivanahuckova ivanahuckova requested a review from matyax June 15, 2023 11:12
Copy link
Contributor

@matyax matyax left a comment

Choose a reason for hiding this comment

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

👏 👏

Copy link
Contributor

@gabor gabor left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , but i'd recommend not doing template-variables in step-field, at least not in the initial version.

@ivanahuckova ivanahuckova enabled auto-merge (squash) June 16, 2023 16:07
@ivanahuckova ivanahuckova merged commit 82c125d into main Jun 16, 2023
14 checks passed
@ivanahuckova ivanahuckova deleted the ivana/loki-step-editor branch June 16, 2023 16:08
This was referenced Jun 19, 2023
LudoVio pushed a commit that referenced this pull request Jun 26, 2023
* Loki: Implement step editor

* Update to keep value

* Remove console.log

* Remove white space

* Update public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilderOptions.tsx

Co-authored-by: Matias Chomicki <matyax@gmail.com>

* Import trim

* Update using of step in split queries

* Add tests

* Add tests

* Remove step interpolation

---------

Co-authored-by: Matias Chomicki <matyax@gmail.com>
harisrozajac pushed a commit that referenced this pull request Jun 30, 2023
* Loki: Implement step editor

* Update to keep value

* Remove console.log

* Remove white space

* Update public/app/plugins/datasource/loki/querybuilder/components/LokiQueryBuilderOptions.tsx

Co-authored-by: Matias Chomicki <matyax@gmail.com>

* Import trim

* Update using of step in split queries

* Add tests

* Add tests

* Remove step interpolation

---------

Co-authored-by: Matias Chomicki <matyax@gmail.com>
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants