[feat][storage] Added the auto creation of Jaeger ILM/ISM policy for ES/OS #6604
Conversation
Signed-off-by: Manik2708 <mehtamanik96@gmail.com>
Signed-off-by: Manik2708 <mehtamanik96@gmail.com>
Signed-off-by: Manik2708 <mehtamanik96@gmail.com>
|
Some notes for the reviewer:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6604 +/- ##
==========================================
- Coverage 96.05% 95.08% -0.98%
==========================================
Files 364 366 +2
Lines 20750 20972 +222
==========================================
+ Hits 19932 19941 +9
- Misses 622 828 +206
- Partials 196 203 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Manik2708 <mehtamanik96@gmail.com>
| @@ -219,18 +222,6 @@ func TestCreateTemplateError(t *testing.T) { | |||
| require.Error(t, err, "template-error") | |||
| } | |||
|
|
|||
| func TestILMDisableTemplateCreation(t *testing.T) { | |||
There was a problem hiding this comment.
There is no need of test now, as template creation is independent of use_ilm now
| var wantbytes []byte | ||
| fileSuffix := fmt.Sprintf("-%d", tt.esVersion) | ||
| wantbytes, err = FIXTURES.ReadFile("fixtures/" + templateName + fileSuffix + ".json") | ||
| require.NoError(t, err) | ||
| want := string(wantbytes) | ||
| assert.Equal(t, want, got) | ||
| require.NoError(t, json.Unmarshal(wantbytes, &wantObj)) |
There was a problem hiding this comment.
Not related to this PR, but changing the mapping was very difficult due to this test. In this test strings are being compared rather than json objects.
| @@ -155,12 +155,14 @@ func (s *storageExt) Start(_ context.Context, host component.Host) error { | |||
| s.telset.Logger, | |||
| ) | |||
| case cfg.Elasticsearch != nil: | |||
| //nolint: contextcheck | |||
There was a problem hiding this comment.
I really don't know how can I pass context or do I need to supress the linter like this only?
| "actions": { | ||
| "rollover": { | ||
| "max_age": "1d" | ||
| } |
There was a problem hiding this comment.
Not added delete phase, currently confused between using delete in policy or max_retain_age in datastream.
Signed-off-by: Manik Mehta <mehtamanik96@gmail.com>
Signed-off-by: Manik2708 <mehtamanik96@gmail.com>
Signed-off-by: Manik2708 <mehtamanik96@gmail.com>
Signed-off-by: Manik2708 <mehtamanik96@gmail.com>
|
I have been studying the change which PR: #6567. So now archive distinction is only on the basis of index prefix (please correct me if I am wrong). Hence we don't need to manage the archive differently here! |
|
correct, no special logic for archiving |
| @@ -0,0 +1,76 @@ | |||
| service: | |||
There was a problem hiding this comment.
why do we need new configs? Let's change the main ones
| } | ||
| ], | ||
| "ism_template": { | ||
| "index_patterns": ["*jaeger-*"], |
There was a problem hiding this comment.
this feels too broad, it could catch unexpected indices this way. I think it should be parameterized with the index prefix of the actual store
| // NewPolicyManager creates the policy manager with appropriate version and prefixedIndexNameWithSeparator. | ||
| // prefixedIndexNameWithSeparator is the prefix with separator. For example if index prefix is jaeger-main | ||
| // and policy manager is called for span indices, then prefixedIndexNameWithSeparator will be jaeger-main-jaeger-span- | ||
| func NewPolicyManager(cl func() es.Client, prefixedIndexNameWithSeparator string) *PolicyManager { |
There was a problem hiding this comment.
why does everyone need to call New & Init? Can't we have just a single public method MaybeCreatePolicy?
| ,"lifecycle": { | ||
| "name": "{{ .ILMPolicyName }}", | ||
| "rollover_alias": "{{ .IndexPrefix }}jaeger-dependencies-write" | ||
| {{- if .IsOpenSearch }} |
There was a problem hiding this comment.
I am not following why any of these templates need to change, please explain
There was a problem hiding this comment.
For ISM Policy this distinction is required. When ISM Policy is applied, we need not to specify the policy name in template but just the rollover alias!
| serviceWriter: serviceOperationStorage.Write, | ||
| spanConverter: dbmodel.NewFromDomain(p.AllTagsAsFields, p.TagKeysAsFields, p.TagDotReplacement), | ||
| spanServiceIndex: getSpanAndServiceIndexFn(p, writeAliasSuffix), | ||
| spanAndServicePrefixFn: p.getSpanAndServicePrefixFn, |
There was a problem hiding this comment.
why does this need to be a lambda?
There was a problem hiding this comment.
Span writer can't access the params and we need the index prefix which can only be accessed by params.
There was a problem hiding this comment.
the params are in scope right here. You argument doesn't stand.
There was a problem hiding this comment.
Ok, are you saying to do it the way like it is done for getSpanAndServiceIndexFn?
There was a problem hiding this comment.
I am saying there is no point in this indirection. The getSpanAndServiceIndexFn has a very good reason to exist because the exact index names were very different depending on the settings. Your indirection always returns the same thing.
| @@ -104,6 +107,25 @@ func (s *SpanWriter) CreateTemplates(spanTemplate, serviceTemplate string, index | |||
| return nil | |||
| } | |||
|
|
|||
| func (s *SpanWriter) InitializePolicyManager() error { | |||
| spanPrefix := s.spanAndServicePrefixFn(spanIndexBaseName) | |||
There was a problem hiding this comment.
why can't you directly call p.IndexPrefix.Apply here?
There was a problem hiding this comment.
Writer can't access params
| @@ -142,6 +142,9 @@ type Configuration struct { | |||
| // latest adaptive sampling probabilities. | |||
| AdaptiveSamplingLookback time.Duration `mapstructure:"adaptive_sampling_lookback"` | |||
| Tags TagsAsFields `mapstructure:"tags_as_fields"` | |||
| // IsOpenSearch stores whether the backend is of opensearch type or not. | |||
| // If kept empty, jaeger will automatically identify the distinction. | |||
| IsOpenSearch bool `mapstructure:"is_open_search"` | |||
There was a problem hiding this comment.
we never needed this, why now? We can automatically determine the backend based on the info it returns from the initial handshake.
| } | ||
| } | ||
| } else { | ||
| policyExists, err := client.IsmPolicyExists(ctx, DefaultIsmPolicy) |
There was a problem hiding this comment.
there is no need for this bifurcation for ILM/ISM at this level,
There was a problem hiding this comment.
Then what is going to be the correct level. I understood your point of creating a single method from client which will automatically determine whether backend is OS/ES and create policy but we need to distinguish between ilm-policy.json and ism-policy.json
There was a problem hiding this comment.
try to minimize bifurcation.
|
|
||
| // writeAliasName returns write alias name of the index | ||
| func (p *PolicyManager) writeAliasName() string { | ||
| return p.prefixedIndexNameWithSeparator + writeAliasSuffix |
There was a problem hiding this comment.
we can reuse IndexPrefix type that provides concatenation capabilities
| return nil | ||
| } | ||
|
|
||
| func (p *PolicyManager) getJaegerIndices(ctx context.Context) ([]client.Index, error) { |
There was a problem hiding this comment.
what is this function doing and why? I thought the whole point of ILM policy was that we don't have to worry about individual indices
| return nil | ||
| } | ||
|
|
||
| func (p *PolicyManager) createIndexIfNotExists(ctx context.Context, index string) error { |
There was a problem hiding this comment.
we never need this before, what changed?
| } | ||
|
|
||
| // CreateIlmPolicy calls the internal XPackIlmPutLifecycle service | ||
| func (c ClientWrapper) CreateIlmPolicy() es.XPackIlmPutLifecycle { |
There was a problem hiding this comment.
The point of this client is to abstract the differences between ES7/ES8/OS. I think there should be just a single function CreatePolicy and let it figure out internally how to implement it.
|
@yurishkuro Suddenly a point came to my mind. Actually I think that this is not the correct approach. (Your questions in the need of
|
|
I think this is what you should've started from - discuss the desired behavior and backwards compatibility, before writing code. Describe user workflows
|
|
@yurishkuro Sorry for the churn, I tried taking inspiration from the init of |
Which problem is this PR solving?
Part of: #4708
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test