-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Jaeger-V2] Add configurable index data layout and rollover frequency #5790
[Jaeger-V2] Add configurable index data layout and rollover frequency #5790
Conversation
5e66765
to
f77bc5b
Compare
0c67510
to
d203d55
Compare
01544c8
to
05d35bc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, thanks for doing this.
e89accf
to
422a678
Compare
9f49a23
to
8d42129
Compare
ad01a13
to
8d3ebb6
Compare
ok,i will check it soon |
Signed-off-by: Jared Tan <[email protected]>
2f587d5
to
037f479
Compare
ES v7 failed with
|
Signed-off-by: Jared Tan <[email protected]>
rollover tests fail - it looks like somewhere the prefix is not being respected. |
Signed-off-by: Jared Tan <[email protected]>
Signed-off-by: Jared Tan <[email protected]>
…ithub.com/JaredTan95/jaeger into configure-index-daltalayout-and-frequency
Signed-off-by: Yuri Shkuro <[email protected]>
another_storage: | ||
elasticsearch: | ||
index_prefix: "jaeger-archive" | ||
indices: | ||
spans: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro my apologies i don't have a lot of context here but how should we proceed with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to check in the code if writing/reading to/from Archive storage relies exclusively on spans index, or if it uses any other indices. If the latter, then we need to override the prefix to jaeger-archive
not just for spans but for other indices too. This is where the defaults:
section I mentioned in another comment would be useful - we could just define it once instead of duplicating all the information.
IndexPrefix: cfg.IndexPrefix, | ||
IndexDateLayout: cfg.IndexDateLayoutDependencies, | ||
IndexPrefix: cfg.Indices.Dependencies.Prefix, | ||
IndexDateLayout: cfg.Indices.Dependencies.RolloverFrequency, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a bug: RolloverFrequency -> DateLayout. Please re-check everywhere.
type IndexTemplateOptions struct { | ||
UseILM bool | ||
ILMPolicyName string | ||
config.IndexOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -20,10 +20,35 @@ extensions: | |||
backends: | |||
some_storage: | |||
elasticsearch: | |||
index_prefix: "jaeger-main" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro is this still something we want to address?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a good usability improvement - perhaps under Indices we can have a "defaults" entry that will apply unless overwritten by the more specific entries like spans:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yurishkuro this PR is already pretty big (31 files changed). thoughts on getting this PR in and tackling this in a follow-up PR under #6059. There's some follow-up work that needs to be done there anyway for aligning the config with OTEL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, it might be a good idea to try out Sapling and do stacked PRs, to avoid precisely this problem (at work we always use stacked PRs)
if prefix != "" { | ||
return prefix + indexPrefixSeparator + index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at mapping.go I think this should be a bit smarter, e.g.
if prefix != "" { | |
return prefix + indexPrefixSeparator + index | |
if prefix != "" { | |
if !prefix.endsWith(indexPrefixSeparator) { prefix += indexPrefixSeparator } | |
return prefix + index |
if prefix != "" { | ||
prefix += indexPrefixSeparator | ||
func getSpanAndServiceIndexFn(archive, useReadWriteAliases bool, spanIndexOpts, serviceIndexOpts cfg.IndexOptions) spanAndServiceIndexFn { | ||
if spanIndexOpts.Prefix != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to above
if spanIndexOpts.Prefix != "" { | |
if spanIndexOpts.Prefix != "" && !strings.HasSuffix(spanIndexOpts.Prefix, indexPrefixSeparator) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 hi! just wanted to check on the status of this. We're pretty close to finishing up #5229 so I wanted to check in on if you had time to address the comments above? Let me know if you'd like me to pick it up for you. |
@mahadzaryab1 hi, I will update in a few days to address the above comments |
Superseded by #6060, let's close this to minimize noise. |
…6060) ## Which problem is this PR solving? - Part of #6059 ## Description of the changes - Continuation of #5790 - Refactors the configurations for ElasticSearch and OpenSearch to simplify the configuration by having more logical groupings ## How was this change tested? - Unit Tests and E2E Integration Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Jared Tan <[email protected]> Signed-off-by: Mahad Zaryab <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Jared Tan <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
…aegertracing#6060) ## Which problem is this PR solving? - Part of jaegertracing#6059 ## Description of the changes - Continuation of jaegertracing#5790 - Refactors the configurations for ElasticSearch and OpenSearch to simplify the configuration by having more logical groupings ## How was this change tested? - Unit Tests and E2E Integration Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Jared Tan <[email protected]> Signed-off-by: Mahad Zaryab <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Jared Tan <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Saumya Shah <[email protected]>
…aegertracing#6060) ## Which problem is this PR solving? - Part of jaegertracing#6059 ## Description of the changes - Continuation of jaegertracing#5790 - Refactors the configurations for ElasticSearch and OpenSearch to simplify the configuration by having more logical groupings ## How was this change tested? - Unit Tests and E2E Integration Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Jared Tan <[email protected]> Signed-off-by: Mahad Zaryab <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Jared Tan <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Saumya Shah <[email protected]>
…aegertracing#6060) ## Which problem is this PR solving? - Part of jaegertracing#6059 ## Description of the changes - Continuation of jaegertracing#5790 - Refactors the configurations for ElasticSearch and OpenSearch to simplify the configuration by having more logical groupings ## How was this change tested? - Unit Tests and E2E Integration Tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Jared Tan <[email protected]> Signed-off-by: Mahad Zaryab <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Jared Tan <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
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~