-
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
Refactor Elasticsearch Storage Configs #5480
Conversation
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5480 +/- ##
=======================================
Coverage 95.52% 95.52%
=======================================
Files 331 331
Lines 16155 16167 +12
=======================================
+ Hits 15432 15444 +12
Misses 548 548
Partials 175 175
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
enabled: false | ||
create_mappings: true |
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.
this looks confusing to me - when something is enabled: false
I don't expect any other properties to make sense. Is create_mappings
even related to aliases?
@@ -46,46 +46,70 @@ import ( | |||
|
|||
// Configuration describes the configuration properties needed to connect to an ElasticSearch cluster | |||
type Configuration struct { |
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 Badger, let's not change the v1 config but create a v2 config. For example, I don' think we need to support everything that v1 supported, specifically I think we should only support ILM and Data Streams.
DateLayoutSpans string `mapstructure:"-"` | ||
DateLayoutServices string `mapstructure:"-"` | ||
DateLayoutSampling string `mapstructure:"-"` | ||
DateLayoutDependencies string `mapstructure:"-"` |
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.
this naming duplication suggests to me that we have some groups of settings that individually apply to different indices (spans, services, sampling, deps). Maybe this top level entities should be the main keys in the config, and internally they can share the same attributes, e.g.
spans:
date_layout:
rollover_frequency:
services:
date_layout:
rollover_frequency:
We could reuse the same struct then for each of the entities.
there is a newer PR #5790 |
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test