[Jaeger-V2] Add configurable index data layout and rollover frequency #5790

JaredTan95 · 2024-07-30T15:06:55Z

make configure index datalayout and frequency in jaeger-v2

I think this way is a bit repetitive, as I am trying to contribute for the first time, there are better ways to achieve please guide~

Part of [jaeger-v2] Rethink components configuration to align with OTEL #5229

cmd/jaeger/config-elasticsearch.yaml

pkg/es/config/config.go

cmd/jaeger/config-elasticsearch.yaml

pkg/es/config/config.go

plugin/storage/es/factory.go

codecov · 2024-08-05T01:37:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.81%. Comparing base (9bdd368) to head (b712a72).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5790      +/-   ##
==========================================
- Coverage   96.82%   96.81%   -0.01%     
==========================================
  Files         345      345              
  Lines       16518    16570      +52     
==========================================
+ Hits        15993    16043      +50     
- Misses        339      340       +1     
- Partials      186      187       +1

Flag	Coverage Δ
badger_v1	`8.02% <0.00%> (-0.01%)`	⬇️
badger_v2	`1.85% <0.00%> (+0.02%)`	⬆️
cassandra-3.x-v1	`16.57% <0.00%> (-0.05%)`	⬇️
cassandra-3.x-v2	`1.78% <0.00%> (+0.02%)`	⬆️
cassandra-4.x-v1	`16.57% <0.00%> (-0.05%)`	⬇️
cassandra-4.x-v2	`1.78% <0.00%> (+0.02%)`	⬆️
elasticsearch-6.x-v1	`19.08% <90.34%> (+0.29%)`	⬆️
elasticsearch-7.x-v1	`19.14% <90.34%> (+0.30%)`	⬆️
elasticsearch-8.x-v1	`19.35% <90.34%> (+0.30%)`	⬆️
elasticsearch-8.x-v2	`1.85% <0.00%> (+0.03%)`	⬆️
grpc_v1	`9.47% <0.00%> (-0.02%)`	⬇️
grpc_v2	`?`
kafka-v1	`9.73% <0.00%> (-0.02%)`	⬇️
kafka-v2	`1.85% <0.00%> (+0.02%)`	⬆️
memory_v2	`1.85% <0.00%> (+0.03%)`	⬆️
opensearch-1.x-v1	`19.20% <90.34%> (+0.31%)`	⬆️
opensearch-2.x-v1	`19.20% <90.34%> (+0.30%)`	⬆️
opensearch-2.x-v2	`?`
tailsampling-processor	`0.49% <0.00%> (+0.03%)`	⬆️
unittests	`95.31% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro

Looks pretty good, thanks for doing this.

cmd/esmapping-generator/app/flags.go

pkg/es/config/config.go

plugin/storage/es/mappings/mapping.go

plugin/storage/es/options.go

cmd/es-rollover/app/init/flags.go

plugin/storage/es/options.go

pkg/es/config/flagutils.go

cmd/es-rollover/app/init/flags_test.go

yurishkuro · 2024-09-01T22:08:46Z

Did you do a manual merge main? You should never do that, either do a rebase main or use GitHub Update Branch. Right now the branch is a mess with a whole bunch of other commits and the DCO linter is going nuts.

One brute force way to fix it is to (a) zip all the files in a repo, (b) recreate the branch off main head, (c) unzip the files back,
(d) commit (it's ok to subsume my commits), (e) force-push the local branch to the existing remote branch.

JaredTan95 · 2024-09-02T12:05:23Z

Did you do a manual merge main? You should never do that, either do a rebase main or use GitHub Update Branch. Right now the branch is a mess with a whole bunch of other commits and the DCO linter is going nuts.

One brute force way to fix it is to (a) zip all the files in a repo, (b) recreate the branch off main head, (c) unzip the files back, (d) commit (it's ok to subsume my commits), (e) force-push the local branch to the existing remote branch.

yes, I did git reabse main locally, maybe a merge conflict, and I was going to do it after you had reviewed it

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

pkg/es/config/config.go

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

yurishkuro · 2024-09-03T17:34:17Z

OS/ES tests specifically are failing, most likely related to your changes.

yurishkuro · 2024-09-03T17:35:13Z

for instance

🚧 🚧 🚧 jaeger-v2 error logs:
  2024/09/03 17:08:50 application version: git-commit=, git-version=, build-date=
  Error: failed to get config: cannot unmarshal the configuration: decoding failed due to the following error(s):
  
  error decoding 'extensions': error reading configuration for "jaeger_storage": decoding failed due to the following error(s):
  
  error decoding 'backends[another_storage]': decoding failed due to the following error(s):
  
  'elasticsearch.indices.spans' has invalid keys: index_prefix
  error decoding 'backends[some_storage]': decoding failed due to the following error(s):
  
  'elasticsearch.indices.spans' has invalid keys: index_prefix
  'elasticsearch.indices.services' has invalid keys: index_prefix
  'elasticsearch.indices.dependencies' has invalid keys: index_prefix
  2024/09/03 17:08:50 failed to get config: cannot unmarshal the configuration: decoding failed due to the following error(s):

JaredTan95 · 2024-09-03T23:09:37Z

for instance


🚧 🚧 🚧 jaeger-v2 error logs:

  2024/09/03 17:08:50 application version: git-commit=, git-version=, build-date=

  Error: failed to get config: cannot unmarshal the configuration: decoding failed due to the following error(s):

  

  error decoding 'extensions': error reading configuration for "jaeger_storage": decoding failed due to the following error(s):

  

  error decoding 'backends[another_storage]': decoding failed due to the following error(s):

  

  'elasticsearch.indices.spans' has invalid keys: index_prefix

  error decoding 'backends[some_storage]': decoding failed due to the following error(s):

  

  'elasticsearch.indices.spans' has invalid keys: index_prefix

  'elasticsearch.indices.services' has invalid keys: index_prefix

  'elasticsearch.indices.dependencies' has invalid keys: index_prefix

  2024/09/03 17:08:50 failed to get config: cannot unmarshal the configuration: decoding failed due to the following error(s):

ok，i will check it soon

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

yurishkuro · 2024-09-06T15:24:35Z

ES v7 failed with

=== RUN   TestIndexRollover_CreateIndicesWithILM/SetPolicyName/WithAdaptiveSampling
Error: template: mapping:5:8: executing "mapping" at <.IndexPrefix>: can't evaluate field IndexPrefix in type *mappings.IndexTemplateOptions

cmd/jaeger/config-elasticsearch.yaml

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

yurishkuro · 2024-09-07T17:52:52Z

rollover tests fail - it looks like somewhere the prefix is not being respected.

Signed-off-by: Jared Tan <jian.tan@daocloud.io>

…ithub.com/JaredTan95/jaeger into configure-index-daltalayout-and-frequency

Signed-off-by: Yuri Shkuro <github@ysh.us>

yurishkuro · 2024-09-08T16:52:36Z

cmd/jaeger/config-elasticsearch.yaml

      another_storage:
        elasticsearch:
-          index_prefix: "jaeger-archive"
+          indices:
+            spans:


Q: is the services index being built at all when the storage is in the archive mode? I don't think it should be, but in this configuration services.prefix: "jaeger-main" would be the default, and the archive storage is not told in any way that it is indeed archive.

yurishkuro · 2024-09-08T16:54:12Z

plugin/storage/es/factory.go

-		IndexPrefix:         cfg.IndexPrefix,
-		IndexDateLayout:     cfg.IndexDateLayoutDependencies,
+		IndexPrefix:         cfg.Indices.Dependencies.Prefix,
+		IndexDateLayout:     cfg.Indices.Dependencies.RolloverFrequency,


looks like a bug: RolloverFrequency -> DateLayout. Please re-check everywhere.

yurishkuro · 2024-09-08T16:59:28Z

plugin/storage/es/mappings/mapping.go

+type IndexTemplateOptions struct {
+	UseILM        bool
+	ILMPolicyName string
+	config.IndexOptions


nit: I would recommend against embedding the whole struct, because it makes the templates tightly coupled with the configuration. E.g. it looks like only a couple of fields are actually used in the templates, prefix and priority, so I would define them explicitly in IndexTemplateOptions and copy from IndexOptions as needed. This way we won't have to change the templates just because we changed the config struct.

yurishkuro · 2024-09-08T17:07:10Z

cmd/jaeger/config-elasticsearch.yaml

@@ -20,10 +20,35 @@ extensions:
    backends:
      some_storage:
        elasticsearch:
-          index_prefix: "jaeger-main"


TODO: we may want to rethink if we still want a single field in the config for index prefix, which would apply if the individual index options didn't override it explicitly. This would simplify the configuration while maintaining the flexibility.

yurishkuro · 2024-09-08T17:09:36Z

plugin/storage/es/spanstore/reader.go

+		if prefix != "" {
+			return prefix + indexPrefixSeparator + index


looking at mapping.go I think this should be a bit smarter, e.g.

Suggested change

if prefix != "" {

return prefix + indexPrefixSeparator + index

if prefix != "" {

if !prefix.endsWith(indexPrefixSeparator) { prefix += indexPrefixSeparator }

return prefix + index

yurishkuro · 2024-09-08T17:12:48Z

plugin/storage/es/spanstore/writer.go

-	if prefix != "" {
-		prefix += indexPrefixSeparator
+func getSpanAndServiceIndexFn(archive, useReadWriteAliases bool, spanIndexOpts, serviceIndexOpts cfg.IndexOptions) spanAndServiceIndexFn {
+	if spanIndexOpts.Prefix != "" {


similar to above

Suggested change

if spanIndexOpts.Prefix != "" {

if spanIndexOpts.Prefix != "" && !strings.HasSuffix(spanIndexOpts.Prefix, indexPrefixSeparator) {

since we do this in multiple places I would extract into a single function. Ideally these kinds of adjustments should be done very early after parsing the config, not spread out across many places in the code. It could be even a helper function on the IndexOptions struct, e.g.

func (o IndexOptions) FullIndexName(name string) { if o.Prefix == "" { return name } if strings.HasSuffix(o.Prefix. indexPrefixSeparator) { return o.Prefix + name } return o.Prefix + indexPrefixSeparator + name }

JaredTan95 requested a review from a team as a code owner July 30, 2024 15:06

JaredTan95 requested a review from albertteoh July 30, 2024 15:06

dosubot bot added storage/elasticsearch v2 labels Jul 30, 2024

JaredTan95 force-pushed the configure-index-daltalayout-and-frequency branch from 5e66765 to f77bc5b Compare July 30, 2024 15:07

JaredTan95 changed the title ~~[Jaeger-V2] configurable index daltalayout and frequency~~ [Jaeger-V2] configurable index datalayout and frequency Jul 30, 2024

JaredTan95 changed the title ~~[Jaeger-V2] configurable index datalayout and frequency~~ [Jaeger-V2] configurable index data layout and frequency Jul 30, 2024

JaredTan95 changed the title ~~[Jaeger-V2] configurable index data layout and frequency~~ [Jaeger-V2] configurable index data layout and rollover frequency Jul 30, 2024

yurishkuro reviewed Jul 31, 2024

View reviewed changes

cmd/jaeger/config-elasticsearch.yaml Outdated Show resolved Hide resolved

JaredTan95 force-pushed the configure-index-daltalayout-and-frequency branch from 0c67510 to d203d55 Compare August 1, 2024 02:59

yurishkuro reviewed Aug 1, 2024

View reviewed changes

pkg/es/config/config.go Outdated Show resolved Hide resolved

yurishkuro reviewed Aug 1, 2024

View reviewed changes

cmd/jaeger/config-elasticsearch.yaml Outdated Show resolved Hide resolved

yurishkuro reviewed Aug 1, 2024

View reviewed changes

cmd/jaeger/config-elasticsearch.yaml Outdated Show resolved Hide resolved

yurishkuro reviewed Aug 1, 2024

View reviewed changes

cmd/jaeger/config-elasticsearch.yaml Outdated Show resolved Hide resolved

yurishkuro reviewed Aug 1, 2024

View reviewed changes

pkg/es/config/config.go Show resolved Hide resolved

JaredTan95 force-pushed the configure-index-daltalayout-and-frequency branch 2 times, most recently from 01544c8 to 05d35bc Compare August 2, 2024 06:02

yurishkuro reviewed Aug 4, 2024

View reviewed changes

pkg/es/config/config.go Outdated Show resolved Hide resolved

pkg/es/config/config.go Outdated Show resolved Hide resolved

plugin/storage/es/factory.go Outdated Show resolved Hide resolved

plugin/storage/es/factory.go Outdated Show resolved Hide resolved

yurishkuro mentioned this pull request Aug 5, 2024

Refactor Elasticsearch Storage Configs #5480

Closed

4 tasks

yurishkuro reviewed Aug 7, 2024

View reviewed changes

JaredTan95 force-pushed the configure-index-daltalayout-and-frequency branch from e89accf to 422a678 Compare August 7, 2024 13:53