Simpler new chunk key v12 #5054

JordanRushing · 2022-01-05T20:05:00Z

What this PR does / why we need it:
This PR branches #4857 and uses a simpler external key structure for chunks.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:
N/A

Checklist

Documentation added
Tests updated
Add an entry in the CHANGELOG.md about the changes.

…nchmark Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com> Fix `TestGrpcStore` to work with new chunk key structure Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com> Add a function to SchemaConfig that returns the correct PeriodConfig for a given time. Signed-off-by: Callum Styan <callumstyan@gmail.com> Add schema config to object client so it we can use the right external key function based on the schema version. Signed-off-by: Callum Styan <callumstyan@gmail.com> Fix failing compactor tests by passing SchemaConfig to chunkClient Remove empty conditional from SchemaForTime Define schema v12; wire-in ChunkPathShardFactor and ChunkPathPeriod as configurable values Add PeriodConfig test steps for new values ChunkPathShardFactor and ChunkPathPeriod in v12 Update test for chunk.NewExternalKey(); remove completed TODO Set defaults for new ChunkPathPeriod and ChunkPathShardFactor SchemaConfig values; change FSObjectClient to use IdentityEncoder instead of Base64Encoder Use IdentityEncoder everywhere we use FSObjectClient for Chunks Add ExternalKey() function to SchemaConfig; update ObjectClient for Chunks

schemaConfig.ExternalKey change. Signed-off-by: Callum Styan <callumstyan@gmail.com>

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

…er parsing it; refactor key tests and benchmarks Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

…ternalKey()` Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

…key-v12

cyriltovena · 2022-01-06T10:27:12Z

pkg/storage/chunk/chunk.go

 		return parseLegacyChunkID(userID, externalKey)
+	} else if strings.Count(externalKey, "/") == 2 { // v12+


This a very hot path. Do we have a benchmark for this function ? If so we can we have a look at the price of the change.

I have some idea of improvement but before I wonder how much the string.Count cost.

Good call! I've fixed an existing benchmark and added two more for the old parsing conditional logic:

$ go test -bench=Key -benchmem goos: darwin goarch: amd64 pkg: github.com/grafana/loki/pkg/storage/chunk cpu: VirtualApple @ 2.50GHz BenchmarkParseNewerExternalKey-8 3107478 412.6 ns/op 0 B/op 0 allocs/op BenchmarkParseNewExternalKey-8 2605483 392.4 ns/op 0 B/op 0 allocs/op BenchmarkParseLegacyExternalKey-8 2919925 414.9 ns/op 48 B/op 1 allocs/op BenchmarkParseOldLegacyExternalKey-8 2884390 412.2 ns/op 48 B/op 1 allocs/op BenchmarkParseOldNewExternalKey-8 3363462 360.0 ns/op 0 B/op 0 allocs/op PASS ok github.com/grafana/loki/pkg/storage/chunk 13.586s

Can you bench compare the root function too

Can you bench compare the root function too

I've added additional benchmarks for the root parsing functions!

Thank you ! If you got some time, there's definitively improvement we can make once this is merged on that root ParseExternalKey. I'm thinking we should try to use only string.Index and reslice as we go to avoid too many string.Index, I think we get it to work with a max of 2 string.Index, instead of string.Index and string.Count (which does a for loop).

We'll talk about this offline, now that we have a benchmark we can compare it, this is not urgent, but something fun you can take on.

docs/sources/upgrading/_index.md

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

KMiller-Grafana

Docs part of this PR are fine now.

cyriltovena

LGTM

I think it's worth trying to optimize the parse function in a follow up pr, to remove those string function call

pkg/storage/chunk/schema_config.go

cyriltovena · 2022-01-06T19:32:42Z

pkg/storage/chunk/chunk.go

 		return parseLegacyChunkID(userID, externalKey)
+	} else if strings.Count(externalKey, "/") == 2 { // v12+


Can you bench compare the root function too

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

memoizes schema number calculations

owen-d · 2022-01-06T21:49:25Z

@cyriltovena I've added 8ee35f8 to this PR.

`ExternalKey()` benches

Diff vs non-memoized version:

benchmark                  old ns/op     new ns/op     delta
BenchmarkExternalKey-4     538           448           -16.88%

benchmark                  old allocs     new allocs     delta
BenchmarkExternalKey-4     7              6              -14.29%

benchmark                  old bytes     new bytes     delta
BenchmarkExternalKey-4     136           112           -17.65%

Diff vs control (tip of main branch):

benchmark                  old ns/op     new ns/op     delta
BenchmarkExternalKey-4     415           448           +7.96%

benchmark                  old allocs     new allocs     delta
BenchmarkExternalKey-4     6              6              +0.00%

benchmark                  old bytes     new bytes     delta
BenchmarkExternalKey-4     112           112           +0.00%

I think it would be much cleaner to have defined our schemas like type Schema int and just had a codec for v12 <> 12, but there's a lot of code to touch there that would further complicate this PR.

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

owen-d and others added 22 commits December 1, 2021 16:32

starts hacking new chunk paths

90fe9c2

Finish plumbing through the chunk.ExternalKey ->

d6b8616

schemaConfig.ExternalKey change. Signed-off-by: Callum Styan <callumstyan@gmail.com>

Clean up lint failures

ebab2ac

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Clean up chunk.go and fix tests

25b631a

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Break SchemaConfig.ExternalKey() into smaller functions

7fda43b

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Quickly fix variable name in SchemaConfig.newerExternalKey()

db54175

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Clean up ExternalKey conditional logic; add better comments; add tests

abee82d

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Fix a bug where we are prepending userID to legacy Chunk keys but nev…

ca95847

…er parsing it; refactor key tests and benchmarks Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Add small SchemaConfig test

b753541

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Update docs and CHANGELOG.md

1cc0986

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Merge branch 'main' into new-chunk-external-key-v12

50911ba

Correctly return an error when failing to parse a chunk external key

1645a49

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Revert IdentityEncoder to Base64Encoder after testing

a3ae0ea

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Merge branch 'main' into new-chunk-external-key-v12

823a63f

Fix assignment in test for linter

2679054

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Remove leftover comments from development

aa3dc28

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Change v12 version comment format; remove redundant login in `ParseEx…

d300f55

…ternalKey()` Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

Change remaining v12 comment style

8b582f6

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

Merge remote-tracking branch 'upstream/main' into new-chunk-external-…

0a089c7

…key-v12

Pass chunk.SchemaConfig to new parallel chunk client

d3d2fc3

Simplify chunk external key prefixes for schema v12

24219ba

pull-request-size bot added the size/XL label Jan 5, 2022

owen-d approved these changes Jan 5, 2022

View reviewed changes

JordanRushing mentioned this pull request Jan 5, 2022

New chunk external key v12 #4857

Closed

3 tasks

JordanRushing self-assigned this Jan 5, 2022

cyriltovena reviewed Jan 6, 2022

View reviewed changes

JordanRushing marked this pull request as ready for review January 6, 2022 19:02

JordanRushing requested a review from KMiller-Grafana as a code owner January 6, 2022 19:02

JordanRushing requested a review from a team as a code owner January 6, 2022 19:02

KMiller-Grafana reviewed Jan 6, 2022

View reviewed changes

docs/sources/upgrading/_index.md Outdated Show resolved Hide resolved

Fix broken benchmark; add benchmarks for old parsing conditional

1d1dab9

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

pull-request-size bot added size/XXL and removed size/XL labels Jan 6, 2022

Remove unneeded lines from upgrading doc

e23c97a

KMiller-Grafana approved these changes Jan 6, 2022

View reviewed changes

cyriltovena approved these changes Jan 6, 2022

View reviewed changes

JordanRushing and others added 3 commits January 6, 2022 14:26

Add benchmarks for root chunk external key parsing functions

9bb4960

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

memoizes schema number calculations

8ee35f8

Merge pull request #3 from owen-d/pull/5054

8c833be

memoizes schema number calculations

Resolve linter issue with PeriodConfig receiver name

fcb10a6

Signed-off-by: Jordan Rushing <jordan.rushing@grafana.com>

owen-d merged commit 291360d into grafana:main Jan 7, 2022

DavidRayner mentioned this pull request Jan 11, 2023

Loki helm chart 2.x to 3.x migration - Log retention issues #7827

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simpler new chunk key v12 #5054

Simpler new chunk key v12 #5054

JordanRushing commented Jan 5, 2022 •

edited

Loading

cyriltovena Jan 6, 2022

JordanRushing Jan 6, 2022

cyriltovena Jan 6, 2022

JordanRushing Jan 6, 2022

cyriltovena Jan 7, 2022

KMiller-Grafana left a comment

cyriltovena left a comment

cyriltovena Jan 6, 2022

owen-d commented Jan 6, 2022 •

edited

Loading

		return parseLegacyChunkID(userID, externalKey)
		} else if strings.Count(externalKey, "/") == 2 { // v12+

Simpler new chunk key v12 #5054

Simpler new chunk key v12 #5054

Conversation

JordanRushing commented Jan 5, 2022 • edited Loading

cyriltovena Jan 6, 2022

Choose a reason for hiding this comment

JordanRushing Jan 6, 2022

Choose a reason for hiding this comment

cyriltovena Jan 6, 2022

Choose a reason for hiding this comment

JordanRushing Jan 6, 2022

Choose a reason for hiding this comment

cyriltovena Jan 7, 2022

Choose a reason for hiding this comment

KMiller-Grafana left a comment

Choose a reason for hiding this comment

cyriltovena left a comment

Choose a reason for hiding this comment

cyriltovena Jan 6, 2022

Choose a reason for hiding this comment

owen-d commented Jan 6, 2022 • edited Loading

ExternalKey() benches

JordanRushing commented Jan 5, 2022 •

edited

Loading

owen-d commented Jan 6, 2022 •

edited

Loading

`ExternalKey()` benches