feat(logql): Support drop labels in logql pipeline #7975

adityacs · 2022-12-20T09:42:21Z

What this PR does / why we need it:
This PR introduces drop stage in logql pipeline.

Which issue(s) this PR fixes:
Fixes #7870, Fixes #7368

Checklist

Reviewed the CONTRIBUTING.md guide
Documentation added
Tests updated
CHANGELOG.md updated
Changes that require user attention or interaction to upgrade are documented in docs/sources/logql/log_queries.md

owen-d

We can already use | __error__="" to ignore errors and | label_format x="{{.__error__}}" to extract errors to new labels, but I'm definitely interested in a way to remove existing labels. It sounds like your label_format_rename_errors is a nice syntactic sugar for a combination of these.

Would it make more sense to start with a | strip <label> <label> operation which could remove labels from the results? I think this would result in cleaner code as well since we wouldn't have to have special cases for the __error__ label besides preventing it from being rewritten (current behavior).

adityacs · 2023-01-05T17:06:59Z

@owen-d

We can already use | error="" to ignore errors and | label_format x="{{.error}}" to extract errors to new labels

__error__="" drops the lines which contains the error. With ignore_errors we are just removing the __error__ label but keeping the line.

With label_format dst={{__error__}}, we can extract but we are trying to remove __error__ label here. Instead of just removing, with label_format_rename_error, we can capture the error with different key.

Would it make more sense to start with a | strip operation which could remove labels from the results? I think this would result in cleaner code as well since we wouldn't have to have special cases for the error label besides preventing it from being rewritten (current behavior).

I am totally ok with this. This would be a nice approach. However, I think we should still support renaming __error__ labels(with label_format or label_format_rename_errors), this would allow user to know there was an error in that line. If the user wants to remove or ignore the errors, we could support strip <label> instead of ignore_errors.

owen-d · 2023-01-05T17:44:09Z

error="" drops the lines which contains the error. With ignore_errors we are just removing the error label but keeping the line.

Good distinction there 👍 .

I think we could achieve what you're suggesting with something like

| label_format dst={{__error__}} | strip __error__

This would be a little more verbose, but it won't have to treat the __error__ label in a special way and could be used for other use cases, which I like. WDYT?

adityacs · 2023-01-05T18:03:48Z

| label_format dst={{__error__}} | strip __error__

Yes, this would do.

Should we say label_remove or label_drop or srtrip_label which would be more intuitive instead of strip?

Also, should we support strip with a match condition i.e. strip labels only if it matches some value? Something like|strip __error__=JSONParserErr.

owen-d · 2023-01-05T19:32:55Z

I'm inclined to use | drop <label> <label> and | drop <label>=<match> <label>=<match>

adityacs · 2023-01-06T06:54:32Z

| drop <label> <label> and | drop <label>=<match> <label>=<match>
Yes, this works. Will make changes accordingly.

grafanabot · 2023-01-10T12:07:02Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

adityacs · 2023-01-10T12:24:12Z

@owen-d Not sure if we should support __error__details__ explicitly. For match expression it would be too verbose to match by __error_details__. For example, something like this would be too verbose |drop __error_details__="expecting json object(6), but it is not".

What do you suggest?

owen-d

I'd like to support drop with __error_details__. Is there a reason you don't think so? For instance, | drop __error_details__=~"unexpected json:.*" would be helpful.

owen-d · 2023-01-10T17:54:55Z

docs/sources/logql/log_queries.md

+- `=~`: regex matches
+- `!~`: regex does not match
+
+The `| drop` expression will drop the given labels in the pipeline. For example, for the query `{job="varlogs"}|json|drop level, method="GET"`, with below log line


Let's add a few other examples:

| drop level | drop level="warn" | drop app=~"api.*" | drop foo="bar", bazz

Have added one more example with regex match. The examples shown in doc will cover the above examples. Let me know if we should add more.

grafanabot · 2023-01-11T09:17:17Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

adityacs · 2023-01-11T11:56:33Z

@owen-d Have added support for __error_details__ in drop

owen-d

This looks great. I'm approving now, but I think | drop __error__ should also remove __error_details__ so we can more ergonomically use | drop __error__ instead of needing to do | drop __error__ __error_details__. Can we make that last change?

owen-d · 2023-01-12T17:25:01Z

docs/sources/logql/log_queries.md

+the result will be
+
+```
+{} INFO GET / loki.net 200


I think this is how it should work, but currently I expect it to keep __error_details__ when | drop __error__ is used. I think it should also remove __error_details__ like this doc suggests.

@owen-d Yeah, that was the earlier behaviour I added but later removed that while adding support for __error__details__. I will send a follow up PR for this change.

What should be the behaviour in this case |drop __error_details__ or |drop __error_details__="some error details"? Should we remove __error__ with this?

Personally I think |drop __error_details__ shouldn't remove __error__, but |drop __error__ should also remove __error_details__. To me, it feels like the __error_details__ are a child of __error__

grafanabot · 2023-01-12T21:46:32Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

adityacs added 5 commits December 16, 2022 23:24

add ignore errors pipeline expr

4b03fe4

add label_format_rename_errors stage to pipeline

0d14453

add tests

80f2733

add docs

9e01cb5

cleanup test

2999a4b

adityacs requested a review from a team as a code owner December 20, 2022 09:42

pull-request-size bot added the size/L label Dec 20, 2022

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Dec 20, 2022

add pipeline tests

4b0bd4f

pull-request-size bot added size/XL and removed size/L labels Dec 27, 2022

adityacs added 5 commits December 27, 2022 14:13

merge main

7f86824

update docs

139a71c

fix lint

a9564e6

update changelog

b969eaf

remove ErrorDetailsLabel check in label_filter

c8304c9

owen-d reviewed Jan 5, 2023

View reviewed changes

adityacs added 2 commits January 6, 2023 12:28

merge main

ff7f64b

delete ignore_errors code

400ceba

pull-request-size bot added size/L and removed size/XL labels Jan 10, 2023

adityacs added 4 commits January 10, 2023 15:15

add droplabel stage

b516b1a

add tests

a376d00

remove label_format_rename_errors code

4accf23

update docs

b86453a

pull-request-size bot added size/XL and removed size/L labels Jan 10, 2023

fix lint errors

a8eb09c

adityacs changed the title ~~feat(logql): add ignore_errors and label_format_rename_errors stages in logql pipeline~~ feat(logql): Support drop labels in logql pipeline Jan 10, 2023

remove label_format_rename_errors code

31b35a1

pull-request-size bot added size/L and removed size/XL labels Jan 10, 2023

adityacs added 2 commits January 10, 2023 17:31

remove label_format_rename_errors code

497038c

update changelog

5845014

adityacs requested a review from owen-d January 10, 2023 12:08

owen-d reviewed Jan 10, 2023

View reviewed changes

adityacs added 4 commits January 11, 2023 12:43

include __error_details__ in drop

e531790

fix tests

5a29998

Merge remote-tracking branch 'upstream/main' into ignore_errors

a632617

update docs

5704c37

adityacs requested a review from owen-d January 11, 2023 11:58

owen-d approved these changes Jan 12, 2023

View reviewed changes

MasslessParticle merged commit 8df5803 into grafana:main Jan 12, 2023

adityacs mentioned this pull request Jan 16, 2023

fix(logql): remove __error_details__ label on drop __error__ or other way around #8150

Merged

3 tasks

svennergr mentioned this pull request Jun 15, 2023

Loki: Implement "drop" and "keep" operations grafana/grafana#70187

Closed

5 tasks

grafanabot mentioned this pull request Feb 23, 2024

chore(release-2.8.x): release 2.8.10 #12049

Closed

emaildanwilson mentioned this pull request Mar 13, 2024

Explore Loki: Failed to load log volume for this query grafana/grafana#84144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logql): Support drop labels in logql pipeline #7975

feat(logql): Support drop labels in logql pipeline #7975

adityacs commented Dec 20, 2022 •

edited

Loading

owen-d left a comment •

edited

Loading

adityacs commented Jan 5, 2023

owen-d commented Jan 5, 2023

adityacs commented Jan 5, 2023 •

edited

Loading

owen-d commented Jan 5, 2023 •

edited

Loading

adityacs commented Jan 6, 2023

grafanabot commented Jan 10, 2023

adityacs commented Jan 10, 2023

owen-d left a comment

owen-d Jan 10, 2023

adityacs Jan 11, 2023 •

edited

Loading

grafanabot commented Jan 11, 2023

adityacs commented Jan 11, 2023 •

edited

Loading

owen-d left a comment

owen-d Jan 12, 2023

adityacs Jan 13, 2023

owen-d Jan 17, 2023

grafanabot commented Jan 12, 2023

feat(logql): Support drop labels in logql pipeline #7975

feat(logql): Support drop labels in logql pipeline #7975

Conversation

adityacs commented Dec 20, 2022 • edited Loading

owen-d left a comment • edited Loading

Choose a reason for hiding this comment

adityacs commented Jan 5, 2023

owen-d commented Jan 5, 2023

adityacs commented Jan 5, 2023 • edited Loading

owen-d commented Jan 5, 2023 • edited Loading

adityacs commented Jan 6, 2023

grafanabot commented Jan 10, 2023

adityacs commented Jan 10, 2023

owen-d left a comment

Choose a reason for hiding this comment

owen-d Jan 10, 2023

Choose a reason for hiding this comment

adityacs Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

grafanabot commented Jan 11, 2023

adityacs commented Jan 11, 2023 • edited Loading

owen-d left a comment

Choose a reason for hiding this comment

owen-d Jan 12, 2023

Choose a reason for hiding this comment

adityacs Jan 13, 2023

Choose a reason for hiding this comment

owen-d Jan 17, 2023

Choose a reason for hiding this comment

grafanabot commented Jan 12, 2023

adityacs commented Dec 20, 2022 •

edited

Loading

owen-d left a comment •

edited

Loading

adityacs commented Jan 5, 2023 •

edited

Loading

owen-d commented Jan 5, 2023 •

edited

Loading

adityacs Jan 11, 2023 •

edited

Loading

adityacs commented Jan 11, 2023 •

edited

Loading