[refactor] Seperated the parts of ES-Rollover Config common with ES Config #6516

Manik2708 · 2025-01-09T09:38:14Z

Which problem is this PR solving?

Supporting PR of #6500

Description of the changes

Comment: [v2]Added Auto Index Rollover #6500 (comment)

How was this change tested?

Unit Tests

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: npm run lint and npm run test

…onfig Signed-off-by: Manik2708 <[email protected]>

codecov · 2025-01-09T09:47:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.28%. Comparing base (980dc31) to head (6e98255).
Report is 12 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6516   +/-   ##
=======================================
  Coverage   96.27%   96.28%           
=======================================
  Files         372      373    +1     
  Lines       21283    21301   +18     
=======================================
+ Hits        20491    20509   +18     
  Misses        605      605           
  Partials      187      187

Flag	Coverage Δ
badger_v1	`10.64% <0.00%> (-0.03%)`	⬇️
badger_v2	`2.77% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v1-manual	`16.53% <0.00%> (-0.05%)`	⬇️
cassandra-4.x-v2-auto	`2.70% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v2-manual	`2.70% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v1-manual	`16.53% <0.00%> (-0.05%)`	⬇️
cassandra-5.x-v2-auto	`2.70% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v2-manual	`2.70% <0.00%> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`20.18% <0.00%> (-0.06%)`	⬇️
elasticsearch-7.x-v1	`20.25% <0.00%> (-0.07%)`	⬇️
elasticsearch-8.x-v1	`20.42% <0.00%> (-0.05%)`	⬇️
elasticsearch-8.x-v2	`2.77% <0.00%> (+<0.01%)`	⬆️
grpc_v1	`12.27% <0.00%> (-0.05%)`	⬇️
grpc_v2	`9.06% <0.00%> (-0.04%)`	⬇️
kafka-3.x-v1	`10.33% <0.00%> (-0.03%)`	⬇️
kafka-3.x-v2	`2.77% <0.00%> (-0.01%)`	⬇️
memory_v2	`2.77% <0.00%> (-0.01%)`	⬇️
opensearch-1.x-v1	`20.30% <0.00%> (-0.05%)`	⬇️
opensearch-2.x-v1	`20.29% <0.00%> (-0.07%)`	⬇️
opensearch-2.x-v2	`2.76% <0.00%> (-0.02%)`	⬇️
tailsampling-processor	`0.51% <0.00%> (-0.01%)`	⬇️
unittests	`95.15% <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.

Signed-off-by: Manik2708 <[email protected]>

pkg/es/config/esrollovercfg.go

pkg/es/config/config.go

yurishkuro · 2025-01-09T16:03:14Z

pkg/es/config/config.go

@@ -202,6 +202,31 @@ type BearerTokenAuthentication struct {
 	AllowFromContext bool `mapstructure:"from_context"`
 }

+type RolloverOptions struct {
+	// Archive if set to true will handle archive indices also
+	Archive bool `mapstructure:"archive"`


we're working on removing the explicit knowledge of "archive" from storage implementations. Is this needed? cc @mahadzaryab1

we're very close to cleaning up archive, so I'd like to hold this PR until we merge #6490 and then do archive differently.

pkg/es/config/config.go

yurishkuro · 2025-01-09T16:04:38Z

pkg/es/config/config.go

+	// Unit is used with lookback to remove indices from read alias e.g, days, weeks, months, years
+	Unit string `mapstructure:"unit"`
+	// UnitCount is the count of units for which look-up is performed
+	UnitCount int `mapstructure:"unit-count"`


do we actually support weeks / months / years? Code ptr?

can we not simply have this as Duration type?

This is just a copy of What used in es-rollover. As the need was not to introduce a major change in rollover but just to include it in binary. So should we change this?

I could raise another PR to see if changing to time.Duration would work

It's ok to have the old command's flags to be a mess, but if we incorporate these into the config which will be used in Jaeger v2 we need to do a clean cut (but backwards compatible).

pkg/es/config/config.go

yurishkuro · 2025-01-09T16:05:54Z

pkg/es/config/config.go

@@ -202,6 +202,31 @@ type BearerTokenAuthentication struct {
 	AllowFromContext bool `mapstructure:"from_context"`


if you introduce these new types, where are they going to be incorporated into the main Config? We can add them now, I would like to see the full config design.

This is going to be encorprated in the config of es.

type Rollover struct { // Enabled if set to true will enable the ES Rollover, which will manage the jaeger indices // automatically without manually running the cron job Enabled bool `mapstructure:"enabled"` // Frequency stores the duration after which this job should be performed periodically. Frequency time.Duration `mapstructure:"frequency"` // LookBackEnabled if set to true will enable the look back process in the rollover job. Look-back process // is very similar to Jaeger ES-Cleaner, if cleaning through only ES-Cleaner is required then this // can be set to false. See this section of documentation for more context: // https://www.jaegertracing.io/docs/2.1/elasticsearch/#index-rollover LookBackEnabled bool `mapstructure:"look_back_enabled"` // LookBackOptions store the configuration which will be executed if look back is enabled LookBackOptions LookBackOptions `mapstructure:"look_back_options"` // RollBackConditions stores the conditions on which index writing should be performed RollBackOptions RollBackOptions `mapstructure:"roll_back_options"` // RolloverOptions store the configuration required for setting up the cron job of rolling over indices RolloverOptions RolloverOptions `mapstructure:"rollover_options"` }

yurishkuro · 2025-01-09T16:06:41Z

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


 // Config holds configuration for index cleaner binary.
 type Config struct {
 	app.Config
-	Conditions string
+	config.RollBackOptions


I am confused by the name "roll back" - what is it we're rolling back? Rollover != rollback

I was also confused for the name. I deduced this name from the description of the flag. RolloverConditions would be alright?

Signed-off-by: Manik2708 <[email protected]>

yurishkuro · 2025-01-12T03:13:26Z

pkg/es/config/esrollovercfg.go

+)
+
+type EsRolloverFlagConfig struct {
+	Prefix string


I am not seeing any usage examples where Prefix is defined to be non-empty. What is it needed?

It is in #6500. The config of es uses a namespace es, so the command needs to be with a prefix.

yurishkuro · 2025-01-12T03:13:44Z

pkg/es/config/esrollovercfg.go

+	Prefix string
+}
+
+func (e EsRolloverFlagConfig) AddFlagsForRolloverOptions(flags *flag.FlagSet) {


Suggested change

func (e EsRolloverFlagConfig) AddFlagsForRolloverOptions(flags *flag.FlagSet) {

func (e EsRolloverFlagConfig) AddFlags(flags *flag.FlagSet) {

yurishkuro · 2025-01-12T03:14:05Z

pkg/es/config/esrollovercfg.go

+	flags.Bool(e.Prefix+flagRolloverAdaptiveSampling, defaultAdaptiveSampling, "Enable rollover for adaptive sampling index")
+}
+
+func (e EsRolloverFlagConfig) InitRolloverOptionsFromViper(v *viper.Viper) RolloverOptions {


Suggested change

func (e EsRolloverFlagConfig) InitRolloverOptionsFromViper(v *viper.Viper) RolloverOptions {

func (e EsRolloverFlagConfig) InitFromViper(v *viper.Viper) RolloverOptions {

yurishkuro · 2025-01-12T03:14:58Z

pkg/es/config/esrollovercfg.go

+	return *r
+}
+
+func (e EsRolloverFlagConfig) AddFlagsForLookBackOptions(flags *flag.FlagSet) {


I don't follow - if these are separate sets of options/flags why do they need to be provided by the same config struct?

I could do in a different struct also but that would be the copy of this same struct. These options/flags doesn't have a prefix but we need to have a prefix when extending the config of es.

yurishkuro · 2025-01-12T03:19:06Z

pkg/es/config/config.go

+	// Unit is used with lookback to remove indices from read alias e.g, days, weeks, months, years
+	Unit string `mapstructure:"unit"`
+	// UnitCount is the count of units for which look-up is performed
+	UnitCount int `mapstructure:"unit-count"`


It's ok to have the old command's flags to be a mess, but if we incorporate these into the config which will be used in Jaeger v2 we need to do a clean cut (but backwards compatible).

yurishkuro · 2025-01-12T03:21:39Z

pkg/es/config/config.go

+	AdaptiveSampling bool `mapstructure:"include_adaptive_sampling"`
+}
+
+// LookBackOptions store the options for a sub-process which is similar to index cleaner of auto rollover of jaeger.


I question why these need to be separate. If I think about the user taking a declarative approach to index rollovers, they would describe the frequency of rotations and how much data to retain, so that the rollover and cleanup are done as a single logical process.

All of these 3 options will be in same common struct of Rollover. Please see this:

type Rollover struct { // Enabled if set to true will enable the ES Rollover, which will manage the jaeger indices // automatically without manually running the cron job Enabled bool `mapstructure:"enabled"` // Frequency stores the duration after which this job should be performed periodically. Frequency time.Duration `mapstructure:"frequency"` // LookBackEnabled if set to true will enable the look back process in the rollover job. Look-back process // is very similar to Jaeger ES-Cleaner, if cleaning through only ES-Cleaner is required then this // can be set to false. See this section of documentation for more context: // https://www.jaegertracing.io/docs/2.1/elasticsearch/#index-rollover LookBackEnabled bool `mapstructure:"look_back_enabled"` // LookBackOptions store the configuration which will be executed if look back is enabled LookBackOptions LookBackOptions `mapstructure:"look_back_options"` // RollBackConditions stores the conditions on which index writing should be performed RollBackOptions RollBackOptions `mapstructure:"roll_back_options"` // RolloverOptions store the configuration required for setting up the cron job of rolling over indices RolloverOptions RolloverOptions `mapstructure:"rollover_options"` }

I have created different structs because we want to keep the processes of cmd/es-rollover decoupled. Creating a single struct will give options of LookBack to Rollover also.

yurishkuro · 2025-01-12T03:22:39Z

pkg/es/config/config.go

+}
+
+// RollOverConditions store the conditions required for the rollover operation in the auto-rollover cron job of jaeger.
+type RollOverConditions struct {


similar to question about LookBackOptions, why is this a separate struct?

yurishkuro · 2025-01-12T03:23:32Z

pkg/es/config/config.go

+// RollOverConditions store the conditions required for the rollover operation in the auto-rollover cron job of jaeger.
+type RollOverConditions struct {
+	// Conditions stores the conditions on which index writing should be performed, for example: "{\"max_age\": \"2d\"}"
+	Conditions string `mapstructure:"conditions"`


who interprets these conditions? Are they sent to Elasticsearch verbatim? if so let's say so in the comments.

yurishkuro · 2025-01-12T03:24:42Z

pkg/es/config/config.go

@@ -202,6 +202,31 @@ type BearerTokenAuthentication struct {
 	AllowFromContext bool `mapstructure:"from_context"`
 }

+type RolloverOptions struct {
+	// Archive if set to true will handle archive indices also
+	Archive bool `mapstructure:"archive"`


we're very close to cleaning up archive, so I'd like to hold this PR until we merge #6490 and then do archive differently.

yurishkuro · 2025-01-12T03:25:43Z

pkg/es/config/esrollovercfg.go

+// Copyright (c) 2025 The Jaeger Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package config


The file can be called simply rollover.go - both es and cfg are implied by the package.

Manik2708 · 2025-01-12T19:58:51Z

@yurishkuro As discussed in the comment: #6500 (comment) and also addressing this comment: #6516 (comment) I am thinking of encorporating the config of Rollover and LookBackinto the common config of es-rollover but that will change the command from ./es-rollover lookback --unit=day to ./es-rollover --unit=day lookback. This eventually is not looking good to me, so which approach should we use:

Encorporating the lookback and rollover to common config and removing the structs of LookBack and Rollover.
Keeping these different structs as they are kept in this PR?

yurishkuro · 2025-01-12T21:49:42Z

Do you see a strong business reason for rollover and lookback to be separate actions? I.e. why would someone want to rollover the indices but not apply the retention policy by cleaning the old ones? Is it feasible that these actions would run at different intervals?

If it makes sense for them to run separately then it would make sense to have separate configs with separate run-frequency settings.

I am also curious about how these apply per-index. We recently upgraded configuration (only accessible via v2, not supported in v1) where each index can have its own rotation etc. settings.

Manik2708 · 2025-01-12T22:01:00Z

Do you see a strong business reason for rollover and lookback to be separate actions? I.e. why would someone want to rollover the indices but not apply the retention policy by cleaning the old ones? Is it feasible that these actions would run at different intervals?

If it makes sense for them to run separately then it would make sense to have separate configs with separate run-frequency settings.

I am also curious about how these apply per-index. We recently upgraded configuration (only accessible via v2, not supported in v1) where each index can have its own rotation etc. settings.

I don't think that seperate rollover frequencies is a good option. We can keep the es-rollover cmd as same, that is leave it on the user when they want to run rollover manually but when integrating with Jaeger binary, I think a common frequency of the cron job as a whole would be more beneficial. I don't see any reason of not running rollover after the init. But as per the blog lookback is an optional step because it is similar to es-index-cleaner so for that we can ask from the user whether to enable look back or not. So if lookback is enabled, periodically this will be the flow: init->rollover->lookback and if it is disabled then init->rollover.

yurishkuro · 2025-01-12T22:27:42Z

I am starting to have second thoughts about this whole thing. Both ES and OS already provide automation for index rollover via ILM/ISM. Why do we need to keep maintaining our own custom scripts in the first place?

Manik2708 · 2025-01-13T05:18:02Z

I am starting to have second thoughts about this whole thing. Both ES and OS already provide automation for index rollover via ILM/ISM. Why do we need to keep maintaining our own custom scripts in the first place?

Is this what are you saying? https://www.elastic.co/guide/en/elasticsearch/reference/current/getting-started-index-lifecycle-management.html

yurishkuro · 2025-01-13T05:21:53Z

Yes (but for OpenSearch there's ISM, which is similar but not the same).

Since you've been reading about these rollovers, can you propose what might be needed from the Jaeger binary if we only wanted to recommend to users to use ILM/ISM?

Manik2708 · 2025-01-13T05:26:51Z

Yes (but for OpenSearch there's ISM, which is similar but not the same).

Since you've been reading about these rollovers, can you propose what might be needed from the Jaeger binary if we only wanted to recommend to users to use ILM/ISM?

Sure, can work on it! Then for now I think I should stop working on this refactoring or using es-rollover in jaeger binary. Let's keep es-rollover as it is and try to use ILM/ISM automatic rollovers in jaeger binary. Am I understanding right?

yurishkuro · 2025-01-13T18:30:37Z

Yes, that's correct. Sorry for the churn, it seemed like a good idea originally, but it turned into more longer term maintenance than I feel it's worth. Jaeger project should be focusing on tracing, not on db management, especially when DBs already support automation out of the box.

Manik2708 · 2025-01-13T21:22:04Z

@yurishkuro Ok, so closing this PR as these changes doesn't make any sense now. Thanks for giving a direction. Will submit a proposal of enabling the automated rollover using ILM/ISM.

[refactor] Seperated the parts of ES-Rollover Config common with ES C…

3b76f4b

…onfig Signed-off-by: Manik2708 <[email protected]>

Manik2708 requested a review from a team as a code owner January 9, 2025 09:38

Manik2708 requested a review from mahadzaryab1 January 9, 2025 09:38

dosubot bot added the storage/elasticsearch label Jan 9, 2025

cleaning

890ea67

Signed-off-by: Manik2708 <[email protected]>

yurishkuro reviewed Jan 9, 2025

View reviewed changes

docs

6e98255

Signed-off-by: Manik2708 <[email protected]>

Manik2708 requested a review from yurishkuro January 9, 2025 20:02

yurishkuro reviewed Jan 12, 2025

View reviewed changes

Manik2708 mentioned this pull request Jan 12, 2025

[v2]Added Auto Index Rollover #6500

Closed

4 tasks

Manik2708 closed this Jan 13, 2025

akstron mentioned this pull request Jan 17, 2025

[WIP] Add ES Auto Index Cleaner #6425

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] Seperated the parts of ES-Rollover Config common with ES Config #6516

[refactor] Seperated the parts of ES-Rollover Config common with ES Config #6516

Manik2708 commented Jan 9, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

yurishkuro Jan 9, 2025

yurishkuro Jan 12, 2025

yurishkuro Jan 9, 2025

Manik2708 Jan 9, 2025

Manik2708 Jan 9, 2025

yurishkuro Jan 12, 2025

yurishkuro Jan 9, 2025

Manik2708 Jan 9, 2025

yurishkuro Jan 9, 2025

Manik2708 Jan 9, 2025

yurishkuro Jan 12, 2025

Manik2708 Jan 12, 2025

yurishkuro Jan 12, 2025

yurishkuro Jan 12, 2025

yurishkuro Jan 12, 2025

Manik2708 Jan 12, 2025

yurishkuro Jan 12, 2025

yurishkuro Jan 12, 2025

Manik2708 Jan 12, 2025

yurishkuro Jan 12, 2025

yurishkuro Jan 12, 2025

yurishkuro Jan 12, 2025

yurishkuro Jan 12, 2025

Manik2708 commented Jan 12, 2025

yurishkuro commented Jan 12, 2025

Manik2708 commented Jan 12, 2025

yurishkuro commented Jan 12, 2025

Manik2708 commented Jan 13, 2025

yurishkuro commented Jan 13, 2025

Manik2708 commented Jan 13, 2025

yurishkuro commented Jan 13, 2025

Manik2708 commented Jan 13, 2025

		@@ -202,6 +202,31 @@ type BearerTokenAuthentication struct {
		AllowFromContext bool `mapstructure:"from_context"`

	func (e EsRolloverFlagConfig) AddFlagsForRolloverOptions(flags *flag.FlagSet) {
	func (e EsRolloverFlagConfig) AddFlags(flags *flag.FlagSet) {

	func (e EsRolloverFlagConfig) InitRolloverOptionsFromViper(v *viper.Viper) RolloverOptions {
	func (e EsRolloverFlagConfig) InitFromViper(v *viper.Viper) RolloverOptions {

[refactor] Seperated the parts of ES-Rollover Config common with ES Config #6516

[refactor] Seperated the parts of ES-Rollover Config common with ES Config #6516

Conversation

Manik2708 commented Jan 9, 2025

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manik2708 commented Jan 12, 2025

yurishkuro commented Jan 12, 2025

Manik2708 commented Jan 12, 2025

yurishkuro commented Jan 12, 2025

Manik2708 commented Jan 13, 2025

yurishkuro commented Jan 13, 2025

Manik2708 commented Jan 13, 2025

yurishkuro commented Jan 13, 2025

Manik2708 commented Jan 13, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading