Metadata to labels result and filtering support #9702

salvacorts · 2023-06-13T12:57:11Z

What this PR does / why we need it:
In #9700, we support encoding and decoding metadata for each entry into the chunks. This PR adds support for returning metadata labels for matching entries in a query to the returned LabelResults. It also supports filtering out logs by metadata labels.

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

salvacorts · 2023-07-19T07:14:50Z

pkg/logql/log/pipeline.go

@@ -23,8 +23,8 @@ type StreamPipeline interface {
 	BaseLabels() LabelsResult
 	// Process processes a log line and returns the transformed line and the labels.
 	// The buffer returned for the log line can be reused on subsequent calls to Process and therefore must be copied.
-	Process(ts int64, line []byte) (resultLine []byte, resultLabels LabelsResult, matches bool)


I was wondering if the nonIndexedLabels argument should be variadic. I decided to make it variadic so we didn't have to refactor all the tests calling Process without any non-indexed labels. I also think it makes sense to be variadic since non-indexed labels are optional. Happy to change my mind tho. Wdyt?

…tor order for entries in the same timestamp is different

…ering

sandeepsukhani

The changes look good to me. I think it would be good to add some tests with non-indexed labels to TestNewLineSampleExtractor and Test_labelSampleExtractor_Extract

…ering

vlad-diachenko · 2023-07-21T14:08:57Z

pkg/chunkenc/memchunk.go

 	si.currTs = ts
 	si.currLine = line
-	si.currMetadataLabels = metaLabels
+	si.currMetadataLabels = nonIndexedLabels


please rename currMetadataLabels to currNonIndexedLabels. maybe in a separate PR

I have a separate PR in the oven renaming all those "metadata" references.

vlad-diachenko · 2023-07-21T14:12:41Z

pkg/logql/log/metrics_extraction.go

@@ -80,22 +80,25 @@ type streamLineSampleExtractor struct {
 	builder *LabelsBuilder
 }

-func (l *streamLineSampleExtractor) Process(ts int64, line []byte) (float64, LabelsResult, bool) {
+func (l *streamLineSampleExtractor) Process(ts int64, line []byte, metadataLabels ...labels.Label) (float64, LabelsResult, bool) {
+	l.builder.Reset()


it changes the logic. previously if stage is NoopStage we return l.LineExtractor(line), l.builder.GroupedLabels(), true that uses values from the builder, otherwise we rest it and continue processing

If I read the code correctly, it should not cause any problems because Reset just clears any buffered changes done on top of base labels.

Reset just clears any buffered changes done on top of base labels.

Correct, that's my understanding as well

vlad-diachenko

Lgtm

sandeepsukhani

I have left just one nit. Good to merge it once it is addressed.

pkg/logql/log/pipeline.go

Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

**What this PR does / why we need it**: In #9700, we support encoding and decoding metadata for each entry into the chunks. This PR adds support for returning metadata labels for matching entries in a query to the returned LabelResults. It also supports filtering out logs by metadata labels. **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) --------- Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com> (cherry picked from commit 1d04cd5)

#10082) **What this PR does / why we need it**: In #9702 we added support for returning non-indexed labels in the labels results. The problem is that non-indexed labels may overwrite stream labels if both are named the same way. This PR fixes this by adding an `_extracted` suffix if the non-indexed label is already present in the stream labels. **Special notes for your reviewer**: **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e)

pull-request-size bot added the size/XL label Jun 13, 2023

salvacorts changed the title ~~Salvacorts/metadata to labels result and filtering~~ Metadata to labels result and filtering support Jun 13, 2023

salvacorts changed the base branch from main to salvacorts/metadata-to-chunk June 26, 2023 12:38

pull-request-size bot added size/L and removed size/XL labels Jun 26, 2023

salvacorts mentioned this pull request Jul 12, 2023

Add non-indexed metadata to chunks #9700

Merged

6 tasks

salvacorts force-pushed the salvacorts/metadata-to-chunk branch from 0fa7568 to 21df77b Compare July 13, 2023 09:39

Base automatically changed from salvacorts/metadata-to-chunk to main July 18, 2023 06:37

pull-request-size bot added size/XXL and removed size/L labels Jul 18, 2023

salvacorts force-pushed the salvacorts/metadata-to-labelsResult-and-filtering branch from accd132 to 2826cee Compare July 18, 2023 10:23

pull-request-size bot added size/L size/XL and removed size/XXL size/L size/XL labels Jul 18, 2023

salvacorts commented Jul 19, 2023

View reviewed changes

salvacorts marked this pull request as ready for review July 19, 2023 07:40

salvacorts requested a review from a team as a code owner July 19, 2023 07:40

salvacorts added 10 commits July 19, 2023 15:11

Add metadata labels to LabelResults and allow filtering

e7d0535

support metric queries

c370b5c

Fix test - now that mettadata is added to the labelResults, the itera…

3821cbd

…tor order for entries in the same timestamp is different

Fix lint issues

790aa6b

Minor fixes after rebase

60f04a7

Comment-out integration testn and fix another test

321cf84

Add more tests

fb6bbd3

Rename argument

1032a62

Add test for memchunk iter and pipeline

c781c75

Imprtove test to check for resulting labels

3bb02da

salvacorts added 7 commits July 19, 2023 15:15

Fix bug in moveNext wrt EOF not being correctly handled

6d457a5

Remove integration test since it's now implemented as unit-test

5716549

Fix lint issues

f3cc48e

Test metric queries with sample iterator

7370d07

More lint fixes

920ec9a

Fix lint issues (finally?)

b410b86

Remove uneeded code after rebase

1e6f49b

salvacorts force-pushed the salvacorts/metadata-to-labelsResult-and-filtering branch from aac6753 to 1e6f49b Compare July 19, 2023 13:18

Merge branch 'main' into salvacorts/metadata-to-labelsResult-and-filt…

cc6ce91

…ering

sandeepsukhani reviewed Jul 20, 2023

View reviewed changes

Add more tests for metrics extractor

ec685d4

pull-request-size bot added size/XL and removed size/L labels Jul 20, 2023

salvacorts requested a review from sandeepsukhani July 20, 2023 10:44

salvacorts added 2 commits July 21, 2023 11:51

Merge branch 'main' into salvacorts/metadata-to-labelsResult-and-filt…

b1f4b36

…ering

Improvements after merge

37e7d8c

vlad-diachenko reviewed Jul 21, 2023

View reviewed changes

vlad-diachenko approved these changes Jul 24, 2023

View reviewed changes

sandeepsukhani approved these changes Jul 24, 2023

View reviewed changes

pkg/logql/log/pipeline.go Outdated Show resolved Hide resolved

Update pkg/logql/log/pipeline.go

ce51e4c

Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

salvacorts merged commit 1d04cd5 into main Jul 24, 2023

salvacorts deleted the salvacorts/metadata-to-labelsResult-and-filtering branch July 24, 2023 08:31

salvacorts mentioned this pull request Jul 27, 2023

Add _extracted to non-indexed labels with same name as stream labels #10082

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metadata to labels result and filtering support #9702

Metadata to labels result and filtering support #9702

salvacorts commented Jun 13, 2023 •

edited

Loading

salvacorts Jul 19, 2023

sandeepsukhani left a comment

vlad-diachenko Jul 21, 2023

salvacorts Jul 24, 2023

vlad-diachenko Jul 21, 2023

sandeepsukhani Jul 24, 2023 •

edited

Loading

salvacorts Jul 24, 2023

vlad-diachenko left a comment

sandeepsukhani left a comment

Metadata to labels result and filtering support #9702

Metadata to labels result and filtering support #9702

Conversation

salvacorts commented Jun 13, 2023 • edited Loading

salvacorts Jul 19, 2023

Choose a reason for hiding this comment

sandeepsukhani left a comment

Choose a reason for hiding this comment

vlad-diachenko Jul 21, 2023

Choose a reason for hiding this comment

salvacorts Jul 24, 2023

Choose a reason for hiding this comment

vlad-diachenko Jul 21, 2023

Choose a reason for hiding this comment

sandeepsukhani Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

salvacorts Jul 24, 2023

Choose a reason for hiding this comment

vlad-diachenko left a comment

Choose a reason for hiding this comment

sandeepsukhani left a comment

Choose a reason for hiding this comment

salvacorts commented Jun 13, 2023 •

edited

Loading

sandeepsukhani Jul 24, 2023 •

edited

Loading