Add non-indexed metadata to chunks #9700

salvacorts · 2023-06-13T11:22:49Z

What this PR does / why we need it:
In #9694 we support adding metadata labels for each entry in the push payload. In this PR we take that metadata and add it to the entries in the chunk. Supporting serialization and deserialization of those metadata labels.

Which issue(s) this PR fixes:

Special notes for your reviewer:
This PR is built on top of #9694, therefore:

Any change to the parent branch should be rebased here.
Once the parent is merged, rebase this branch to main and delete the commits from the parent branch.

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

owen-d

Skimmed, approach looks good to me. Left a few comments and happy to give a closer review+approval when you're ready. Would like to see some refactoring in bufferedIterator.moveNext if possible.

owen-d · 2023-07-10T19:05:40Z

pkg/chunkenc/memchunk.go

+				BytesBufferPool.Put(si.metaLabelsBuf[i])
+			}
+		}
+		si.metaLabelsBuf = make([][]byte, nLabels*2)


See how the other pools are created and use the "github.com/prometheus/prometheus/util/pool" pkg, which supports different sub-pools by length categories. That allows better reuse than creating a slice here.

Done.

I set a maximum size of 256, i.e. max of 128 meta labels. That should be more than enough, but IIUC, we should now set a hard limit on this and refuse payloads with entries containing more than 128 meta-labels. Alternatively, instead of adding a limit and if we ever hit this maximum, we can increase the pool size. Wdyt?

owen-d · 2023-07-10T19:07:44Z

pkg/chunkenc/memchunk.go

+		return ts, si.buf[:lineSize], nil, true
+	}
+
+	// TODO: This is pretty similar to how we read the line size, and the metadata name and value sizes


Agreed -- this looks like it could all be one helper function which is nice considering how complex this code is.

I think we should do it on a separate PR since:

The changes to extract these to a separate function are not as simple as copy-pasting the code to a separate function. How we read the timestamp and line size is slightly different from how we read the number of labels and the length of the label names and values.

To make sure our refactoring doesn't worsen the performance, we'd need to run some benchmarks. That'll require time and this PR is blocking Metadata to labels result and filtering support #9702

Sandeep will work on implementing string interning inside the chunks for the metadata labels. That'll change this moveNext method so I don't think it's with refactoring it until we make those changes.

I've created this task to keep track of this optimization: https://github.com/grafana/loki-private/issues/792

…at `unorderedWithMetadataHeadBlockFmt` that can work with metadata labels. Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

vlad-diachenko

LGTM ;)

sandeepsukhani

Overall the changes look great to me. Left some minor suggestions and a comment which might need input from other folks.

pkg/chunkenc/memchunk.go

pkg/chunkenc/unordered.go

sandeepsukhani · 2023-07-14T06:43:05Z

pkg/chunkenc/unordered.go

@@ -120,14 +131,14 @@ func (hb *unorderedHeadBlock) Append(ts int64, line string) error {
 		// entries at the same time with the same content, iterate through any existing
 		// entries and ignore the line if we already have an entry with the same content
 		for _, et := range displaced[0].(*nsEntries).entries {
-			if et == line {
+			if et.line == line {


Should we also check for equality of metaLabels here to allow entries with the same timestamp and logline but different non-indexed labels? However, this would mean that we will have to generate a hash with both log line and non-indexed labels in SampleIterator, which uses it for deduping duplicate data due to replication.

It would be rare that someone would have log entries with the same timestamp and logline but different non-indexed labels, so I think we should keep it as is to avoid paying the cost to support this rare case. Just putting it out to see if anyone wants to share their thoughts here. If we decide not to support it, we should make it clear in the docs.

It would be rare that someone would have log entries with the same timestamp and logline but different non-indexed labels

Taking promtail as an example, I don't see any stage that can duplicate log lines. Therefore, this situation would only happen if the user is using a custom client that somehow is running into this scenario. IMO, to avoid having to compute a hash over the meta labels, and to keep the code simple, we should not check the non-indexed labels here.

Taking promtail as an example, I don't see any stage that can duplicate log lines.

I was only considering the case where the same logline is being emitted by some jobs, and for whatever reason, they all have the same stream labels.

I'm ok with just checking (indexed_labels, ts, line) for equality here without specifying a formal stance yet. We can later choose to handle this differently if we choose as long as it doesn't seriously break backwards compatibility in the future. Basically, before we include this in a release

I am inclined towards keeping it as is to avoid supporting it partially, i.e. without taking care of required changes in SampleIterator. If you strongly feel we should make this change, please let me know, and I will take care of it with my string interning change.

sandeepsukhani · 2023-07-14T07:01:39Z

pkg/chunkenc/unordered.go

+
+			if hb.format >= UnorderedWithMetadataHeadBlockFmt {
+				// Serialize metadata labels
+				n = binary.PutUvarint(encBuf, uint64(len(metaLabels)))


We had decided to write the length of the whole nonIndexedLabels section first so that we can choose to skip it altogether and jump to the beginning of the next entry. I can take care of it while working on string interning.

Good point. We can do it here, but we'd need to iterate through all the labels and sum up their lengths. With strings interning, IIUC, the label and values would be refs to wherever the actual string is. Therefore, in that case we wouldn't need to iterate through all the label value pairs, but rather multiply len(metaLabels) by twice the ref size, right?

In that case, I think it would make more sense to do it in the PR for strings interning directly? To not complicate this PR more than necessary provided that this part would change. Wdyt?

Since we are writing data with variable length encoding, the number of bytes can't be estimated beforehand. We will have to write the data to a temp buffer first, calculate and write its size first and then write the data from temp buffer. I will take care of it while working on string interning.

Keep in mind, this will break compatibility between this PR and a future PR unless we add a newer format than UnorderedWithMetadataHeadBlockFmt. This is one of the benefits of not setting this as the default yet (via DefaultHeadBlockFmt = UnorderedHeadBlockFmt) -- we can change the implementation in a later PR before we start using it.

sandeepsukhani

Left some minor nits, mostly around tests. I think we can merge this once they get addressed. Nice work!

pkg/chunkenc/memchunk_test.go

pkg/chunkenc/unordered.go

pkg/chunkenc/unordered_test.go

sandeepsukhani

Left 2 minor suggestions. Overall it LGTM.

sandeepsukhani · 2023-07-17T10:44:18Z

pkg/chunkenc/unordered_test.go

+		rt:     (recovered.(*unorderedHeadBlock)).rt,
+		lines:  (recovered.(*unorderedHeadBlock)).lines,
+		size:   (recovered.(*unorderedHeadBlock)).size,
+		mint:   (recovered.(*unorderedHeadBlock)).mint,
+		maxt:   (recovered.(*unorderedHeadBlock)).maxt,


I think we should be using data from unordered to build the expected value, right?
Otherwise, we won't catch any bugs since we are mostly comparing the same values.

Absolutely. Done. Thank you!

sandeepsukhani · 2023-07-17T10:44:27Z

pkg/chunkenc/unordered_test.go

+		rt:     (recovered.(*unorderedHeadBlock)).rt,
+		lines:  (recovered.(*unorderedHeadBlock)).lines,
+		size:   (recovered.(*unorderedHeadBlock)).size,
+		mint:   (recovered.(*unorderedHeadBlock)).mint,
+		maxt:   (recovered.(*unorderedHeadBlock)).maxt,


owen-d

Nice work, lgtm

owen-d · 2023-07-17T21:26:07Z

pkg/chunkenc/unordered.go

@@ -120,14 +131,14 @@ func (hb *unorderedHeadBlock) Append(ts int64, line string) error {
 		// entries at the same time with the same content, iterate through any existing
 		// entries and ignore the line if we already have an entry with the same content
 		for _, et := range displaced[0].(*nsEntries).entries {
-			if et == line {
+			if et.line == line {


I'm ok with just checking (indexed_labels, ts, line) for equality here without specifying a formal stance yet. We can later choose to handle this differently if we choose as long as it doesn't seriously break backwards compatibility in the future. Basically, before we include this in a release

owen-d · 2023-07-17T21:31:11Z

pkg/chunkenc/unordered.go

+
+			if hb.format >= UnorderedWithMetadataHeadBlockFmt {
+				// Serialize metadata labels
+				n = binary.PutUvarint(encBuf, uint64(len(metaLabels)))


Keep in mind, this will break compatibility between this PR and a future PR unless we add a newer format than UnorderedWithMetadataHeadBlockFmt. This is one of the benefits of not setting this as the default yet (via DefaultHeadBlockFmt = UnorderedHeadBlockFmt) -- we can change the implementation in a later PR before we start using it.

**What this PR does / why we need it**: In #9700, we added support for writing non-indexed labels from entries into chunks. This PR introduced two bugs: - When the buffered iterator is closed, the buffer to read metadata labels is put back into the pool but not set to nil. Subsequent uses of the iterator may write on top of the buffer that was put back on the pool. This may lead to inconsistent/incorrect results. - Inside the buffered iterator, we were not correctly handling EOFs while reading the number of labels and each label length. This was due to the `lastAttemp` variable not being reset. This PR adds a new test for these two bugs and also fixes them. **Which issue(s) this PR fixes**: Fixes #9700 **Special notes for your reviewer**: **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [ ] 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/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)

**What this PR does / why we need it**: In #9700, we support encoding and decoding metadata for each entry into the chunks. In this PR we: - Update the bytes processed stats to account for the bytes from those non-indexed labels - Add new stats for bytes processed for those non-indexed labels - Add new ingestion metrics to track ingested non-indexed bytes **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/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)

**What this PR does / why we need it**: In grafana#9700, we support encoding and decoding metadata for each entry into the chunks. In this PR we: - Update the bytes processed stats to account for the bytes from those non-indexed labels - Add new stats for bytes processed for those non-indexed labels - Add new ingestion metrics to track ingested non-indexed bytes **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/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](grafana@d10549e)

**What this PR does / why we need it**: In #9700, we support encoding and decoding non-indexed labels for each entry into the chunks. This PR adds support for writing/reading the non-indexed labels to/from the WAL. **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 - [ ] 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/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)

**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>

…and processing non-indexed labels (#10044) **What this PR does / why we need it**: In PR #9700, we added support for storing non-indexed labels in chunks. This PR optimizes resource usage for storing and processing non-indexed labels by doing string interning. We will store deduped label names and values as a list in chunks in the newly added non-indexed labels section. The labels would then be referenced in blocks by their index(called symbols). Additionally, I have started the convention of writing lengths of sections with their offsets within chunks, making it easier to introduce new sections. The section offsets and lengths would be stored at the end of the chunk, similar to [TOC](https://ganeshvernekar.com/blog/prometheus-tsdb-persistent-block-and-its-index/#a-toc) in TSDB. **Checklist** - [x] Tests updated

**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)

…and processing non-indexed labels (#10044) **What this PR does / why we need it**: In PR #9700, we added support for storing non-indexed labels in chunks. This PR optimizes resource usage for storing and processing non-indexed labels by doing string interning. We will store deduped label names and values as a list in chunks in the newly added non-indexed labels section. The labels would then be referenced in blocks by their index(called symbols). Additionally, I have started the convention of writing lengths of sections with their offsets within chunks, making it easier to introduce new sections. The section offsets and lengths would be stored at the end of the chunk, similar to [TOC](https://ganeshvernekar.com/blog/prometheus-tsdb-persistent-block-and-its-index/#a-toc) in TSDB. **Checklist** - [x] Tests updated (cherry picked from commit 9b554bb)

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

salvacorts changed the base branch from main to salvacorts/metadata-push-payload June 13, 2023 11:23

salvacorts mentioned this pull request Jun 27, 2023

Metadata to labels result and filtering support #9702

Merged

6 tasks

vlad-diachenko mentioned this pull request Jul 1, 2023

added ability to specify head block format version #9841

Closed

7 tasks

owen-d reviewed Jul 10, 2023

View reviewed changes

Base automatically changed from salvacorts/metadata-push-payload to main July 13, 2023 07:13

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

salvacorts and others added 8 commits July 13, 2023 09:37

Add metadata to chunks

a7d0ff8

Empty-Commit - Force CI run

a1dc712

Fix lint and fmt issues

db14bad

Fix more lint issues

089a643

Fix more lint issues

db4443d

reverted metadata labels from orderedHeadBlock and added a new form…

95fe0d4

…at `unorderedWithMetadataHeadBlockFmt` that can work with metadata labels. Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>

Use pool bor buffer matrix

4c78a5d

Fix tests and naming after rebase upstream

21df77b

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

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

Fix lint issues

9d1b5bc

salvacorts marked this pull request as ready for review July 13, 2023 10:25

salvacorts requested a review from a team as a code owner July 13, 2023 10:25

vlad-diachenko approved these changes Jul 14, 2023

View reviewed changes

sandeepsukhani reviewed Jul 14, 2023

View reviewed changes

salvacorts added 2 commits July 14, 2023 12:04

Put labels buff back to pool when closing iterator

8c105b5

Merge branch 'main' into salvacorts/metadata-to-chunk

e80481e

sandeepsukhani requested changes Jul 17, 2023

View reviewed changes

salvacorts added 2 commits July 17, 2023 09:14

Cast logproto.LabelsAdapter to labels.Labels and viceversa

1df541d

Addess PR feeback and add/modify tests and benchmarks

ca8f1b4

pull-request-size bot removed the size/XL label Jul 17, 2023

pull-request-size bot added the size/XXL label Jul 17, 2023

Fix lint issues

0d5816f

salvacorts requested a review from sandeepsukhani July 17, 2023 10:16

sandeepsukhani approved these changes Jul 17, 2023

View reviewed changes

Compare against unordered

8fe4f4f

owen-d approved these changes Jul 17, 2023

View reviewed changes

sandeepsukhani merged commit ce91076 into main Jul 18, 2023

sandeepsukhani deleted the salvacorts/metadata-to-chunk branch July 18, 2023 06:37

salvacorts mentioned this pull request Jul 19, 2023

Fix buffered iterator #9976

Merged

7 tasks

salvacorts mentioned this pull request Jul 20, 2023

Improve observability for non-indexed labels usage #9993

Merged

7 tasks

salvacorts mentioned this pull request Jul 21, 2023

Support non-indexed labels in WAL #10011

Merged

7 tasks

sandeepsukhani mentioned this pull request Jul 24, 2023

implementing string interning to optimize resource usage for storing and processing non-indexed labels #10044

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add non-indexed metadata to chunks #9700

Add non-indexed metadata to chunks #9700

salvacorts commented Jun 13, 2023 •

edited by sandeepsukhani

Loading

owen-d left a comment

owen-d Jul 10, 2023

salvacorts Jul 12, 2023 •

edited

Loading

owen-d Jul 10, 2023

salvacorts Jul 12, 2023

vlad-diachenko left a comment

sandeepsukhani left a comment

sandeepsukhani Jul 14, 2023 •

edited

Loading

salvacorts Jul 14, 2023

sandeepsukhani Jul 14, 2023

owen-d Jul 17, 2023

sandeepsukhani Jul 18, 2023

sandeepsukhani Jul 14, 2023

salvacorts Jul 14, 2023

sandeepsukhani Jul 17, 2023

owen-d Jul 17, 2023

sandeepsukhani left a comment

sandeepsukhani left a comment

sandeepsukhani Jul 17, 2023

salvacorts Jul 17, 2023

sandeepsukhani Jul 17, 2023

salvacorts Jul 17, 2023

owen-d left a comment

owen-d Jul 17, 2023

owen-d Jul 17, 2023

Add non-indexed metadata to chunks #9700

Add non-indexed metadata to chunks #9700

Conversation

salvacorts commented Jun 13, 2023 • edited by sandeepsukhani Loading

owen-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvacorts Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

sandeepsukhani Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandeepsukhani left a comment

Choose a reason for hiding this comment

sandeepsukhani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owen-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salvacorts commented Jun 13, 2023 •

edited by sandeepsukhani

Loading

salvacorts Jul 12, 2023 •

edited

Loading

sandeepsukhani Jul 14, 2023 •

edited

Loading