Skip to content
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.

Sign up for GitHub

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] Seperated the parts of ES-Rollover Config common with ES Config #6516

Closed
wants to merge 3 commits into from
+221 −85
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b76f4b
[refactor] Seperated the parts of ES-Rollover Config common with ES C…
Manik2708 Jan 9, 2025
890ea67
cleaning
Manik2708 Jan 9, 2025
6e98255
docs
Manik2708 Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 18 additions & 31 deletions cmd/es-rollover/app/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,65 +10,52 @@ import (
"go.opentelemetry.io/collector/config/configtls"

"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
"github.com/jaegertracing/jaeger/pkg/es/config"
)

var tlsFlagsCfg = tlscfg.ClientFlagsConfig{Prefix: "es"}
var (
tlsFlagsCfg = tlscfg.ClientFlagsConfig{Prefix: "es"}
esrolloverCfg = config.EsRolloverFlagConfig{}
)

const (
indexPrefix = "index-prefix"
archive = "archive"
username = "es.username"
password = "es.password"
useILM = "es.use-ilm"
ilmPolicyName = "es.ilm-policy-name"
timeout = "timeout"
skipDependencies = "skip-dependencies"
adaptiveSampling = "adaptive-sampling"
indexPrefix = "index-prefix"
username = "es.username"
password = "es.password"
useILM = "es.use-ilm"
)

// Config holds the global configurations for the es rollover, common to all actions
type Config struct {
IndexPrefix string
Archive bool
Username string
Password string
TLSEnabled bool
ILMPolicyName string
UseILM bool
Timeout int
SkipDependencies bool
AdaptiveSampling bool
TLSConfig configtls.ClientConfig
config.RolloverOptions
IndexPrefix string
Username string
Password string
TLSEnabled bool
UseILM bool
TLSConfig configtls.ClientConfig
}

// AddFlags adds flags
func AddFlags(flags *flag.FlagSet) {
esrolloverCfg.AddFlagsForRolloverOptions(flags)
flags.String(indexPrefix, "", "Index prefix")
flags.Bool(archive, false, "Handle archive indices")
flags.String(username, "", "The username required by storage")
flags.String(password, "", "The password required by storage")
flags.Bool(useILM, false, "Use ILM to manage jaeger indices")
flags.String(ilmPolicyName, "jaeger-ilm-policy", "The name of the ILM policy to use if ILM is active")
flags.Int(timeout, 120, "Number of seconds to wait for master node response")
flags.Bool(skipDependencies, false, "Disable rollover for dependencies index")
flags.Bool(adaptiveSampling, false, "Enable rollover for adaptive sampling index")
tlsFlagsCfg.AddFlags(flags)
}

// InitFromViper initializes config from viper.Viper.
func (c *Config) InitFromViper(v *viper.Viper) {
c.RolloverOptions = esrolloverCfg.InitRolloverOptionsFromViper(v)
c.IndexPrefix = v.GetString(indexPrefix)
if c.IndexPrefix != "" {
c.IndexPrefix += "-"
}
c.Archive = v.GetBool(archive)
c.Username = v.GetString(username)
c.Password = v.GetString(password)
c.ILMPolicyName = v.GetString(ilmPolicyName)
c.UseILM = v.GetBool(useILM)
c.Timeout = v.GetInt(timeout)
c.SkipDependencies = v.GetBool(skipDependencies)
c.AdaptiveSampling = v.GetBool(adaptiveSampling)
tlsCfg, err := tlsFlagsCfg.InitFromViper(v)
if err != nil {
panic(err)
Expand Down
40 changes: 19 additions & 21 deletions cmd/es-rollover/app/init/action_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/jaegertracing/jaeger/cmd/es-rollover/app"
"github.com/jaegertracing/jaeger/pkg/es/client"
"github.com/jaegertracing/jaeger/pkg/es/client/mocks"
"github.com/jaegertracing/jaeger/pkg/es/config"
)

func TestIndexCreateIfNotExist(t *testing.T) {
Expand Down Expand Up @@ -90,8 +91,8 @@ func TestRolloverAction(t *testing.T) {
},
config: Config{
Config: app.Config{
Archive: true,
UseILM: true,
RolloverOptions: config.RolloverOptions{Archive: true},
UseILM: true,
},
},
expectedErr: errors.New("ILM is supported only for ES version 7+"),
Expand All @@ -104,8 +105,8 @@ func TestRolloverAction(t *testing.T) {
expectedErr: errors.New("version error"),
config: Config{
Config: app.Config{
Archive: true,
UseILM: true,
RolloverOptions: config.RolloverOptions{Archive: true},
UseILM: true,
},
},
},
Expand All @@ -118,9 +119,8 @@ func TestRolloverAction(t *testing.T) {
expectedErr: errors.New("ILM policy myilmpolicy doesn't exist in Elasticsearch. Please create it and re-run init"),
config: Config{
Config: app.Config{
Archive: true,
UseILM: true,
ILMPolicyName: "myilmpolicy",
RolloverOptions: config.RolloverOptions{Archive: true, ILMPolicyName: "myilmpolicy"},
UseILM: true,
},
},
},
Expand All @@ -133,9 +133,8 @@ func TestRolloverAction(t *testing.T) {
expectedErr: errors.New("error getting ilm policy"),
config: Config{
Config: app.Config{
Archive: true,
UseILM: true,
ILMPolicyName: "myilmpolicy",
RolloverOptions: config.RolloverOptions{Archive: true, ILMPolicyName: "myilmpolicy"},
UseILM: true,
},
},
},
Expand All @@ -148,8 +147,8 @@ func TestRolloverAction(t *testing.T) {
expectedErr: errors.New("error creating template"),
config: Config{
Config: app.Config{
Archive: true,
UseILM: false,
RolloverOptions: config.RolloverOptions{Archive: true},
UseILM: false,
},
},
},
Expand All @@ -164,8 +163,8 @@ func TestRolloverAction(t *testing.T) {
expectedErr: errors.New("error getting jaeger indices"),
config: Config{
Config: app.Config{
Archive: true,
UseILM: false,
RolloverOptions: config.RolloverOptions{Archive: true},
UseILM: false,
},
},
},
Expand All @@ -184,8 +183,8 @@ func TestRolloverAction(t *testing.T) {
expectedErr: errors.New("error creating aliases"),
config: Config{
Config: app.Config{
Archive: true,
UseILM: false,
RolloverOptions: config.RolloverOptions{Archive: true},
UseILM: false,
},
},
},
Expand All @@ -204,8 +203,8 @@ func TestRolloverAction(t *testing.T) {
expectedErr: nil,
config: Config{
Config: app.Config{
Archive: true,
UseILM: false,
RolloverOptions: config.RolloverOptions{Archive: true},
UseILM: false,
},
},
},
Expand All @@ -225,9 +224,8 @@ func TestRolloverAction(t *testing.T) {
expectedErr: nil,
config: Config{
Config: app.Config{
Archive: true,
UseILM: true,
ILMPolicyName: "jaeger-ilm",
RolloverOptions: config.RolloverOptions{Archive: true, ILMPolicyName: "jaeger-ilm"},
UseILM: true,
},
},
},
Expand Down
31 changes: 19 additions & 12 deletions cmd/es-rollover/app/lookback/action_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/jaegertracing/jaeger/cmd/es-rollover/app"
"github.com/jaegertracing/jaeger/pkg/es/client"
"github.com/jaegertracing/jaeger/pkg/es/client/mocks"
"github.com/jaegertracing/jaeger/pkg/es/config"
)

func TestLookBackAction(t *testing.T) {
Expand Down Expand Up @@ -81,11 +82,13 @@ func TestLookBackAction(t *testing.T) {
}).Return(nil)
},
config: Config{
Unit: "days",
UnitCount: 1,
LookBackOptions: config.LookBackOptions{
Unit: "days",
UnitCount: 1,
},
Config: app.Config{
Archive: true,
UseILM: true,
RolloverOptions: config.RolloverOptions{Archive: true},
UseILM: true,
},
},
expectedErr: nil,
Expand All @@ -96,11 +99,13 @@ func TestLookBackAction(t *testing.T) {
indexClient.On("GetJaegerIndices", "").Return(indices, errors.New("get indices error"))
},
config: Config{
Unit: "days",
UnitCount: 1,
LookBackOptions: config.LookBackOptions{
Unit: "days",
UnitCount: 1,
},
Config: app.Config{
Archive: true,
UseILM: true,
RolloverOptions: config.RolloverOptions{Archive: true},
UseILM: true,
},
},
expectedErr: errors.New("get indices error"),
Expand All @@ -111,11 +116,13 @@ func TestLookBackAction(t *testing.T) {
indexClient.On("GetJaegerIndices", "").Return([]client.Index{}, nil)
},
config: Config{
Unit: "days",
UnitCount: 1,
LookBackOptions: config.LookBackOptions{
Unit: "days",
UnitCount: 1,
},
Config: app.Config{
Archive: true,
UseILM: true,
RolloverOptions: config.RolloverOptions{Archive: true},
UseILM: true,
},
},
expectedErr: nil,
Expand Down
17 changes: 5 additions & 12 deletions cmd/es-rollover/app/lookback/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,30 +9,23 @@ import (
"github.com/spf13/viper"

"github.com/jaegertracing/jaeger/cmd/es-rollover/app"
"github.com/jaegertracing/jaeger/pkg/es/config"
)

const (
unit = "unit"
unitCount = "unit-count"
defaultUnit = "days"
defaultUnitCount = 1
)
var esrolloverCfg = config.EsRolloverFlagConfig{}

// Config holds configuration for index cleaner binary.
type Config struct {
app.Config
Unit string
UnitCount int
config.LookBackOptions
}

// AddFlags adds flags for TLS to the FlagSet.
func (*Config) AddFlags(flags *flag.FlagSet) {
flags.String(unit, defaultUnit, "used with lookback to remove indices from read alias e.g, days, weeks, months, years")
flags.Int(unitCount, defaultUnitCount, "count of UNITs")
esrolloverCfg.AddFlagsForLookBackOptions(flags)
}

// InitFromViper initializes config from viper.Viper.
func (c *Config) InitFromViper(v *viper.Viper) {
c.Unit = v.GetString(unit)
c.UnitCount = v.GetInt(unitCount)
c.LookBackOptions = esrolloverCfg.InitLookBackFromViper(v)
}
5 changes: 3 additions & 2 deletions cmd/es-rollover/app/rollover/action_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/jaegertracing/jaeger/cmd/es-rollover/app"
"github.com/jaegertracing/jaeger/pkg/es/client"
"github.com/jaegertracing/jaeger/pkg/es/client/mocks"
"github.com/jaegertracing/jaeger/pkg/es/config"
)

func TestRolloverAction(t *testing.T) {
Expand Down Expand Up @@ -115,9 +116,9 @@ func TestRolloverAction(t *testing.T) {

rolloverAction := Action{
Config: Config{
Conditions: test.conditions,
RollOverConditions: config.RollOverConditions{Conditions: test.conditions},
Config: app.Config{
Archive: true,
RolloverOptions: config.RolloverOptions{Archive: true},
},
},
IndicesClient: indexClient,
Expand Down
12 changes: 5 additions & 7 deletions cmd/es-rollover/app/rollover/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,23 @@ import (
"github.com/spf13/viper"

"github.com/jaegertracing/jaeger/cmd/es-rollover/app"
"github.com/jaegertracing/jaeger/pkg/es/config"
)

const (
conditions = "conditions"
defaultRollbackCondition = "{\"max_age\": \"2d\"}"
)
var esrolloverCfg = config.EsRolloverFlagConfig{}

// Config holds configuration for index cleaner binary.
type Config struct {
app.Config
Conditions string
config.RollOverConditions
}

// AddFlags adds flags for TLS to the FlagSet.
func (*Config) AddFlags(flags *flag.FlagSet) {
flags.String(conditions, defaultRollbackCondition, "conditions used to rollover to a new write index")
esrolloverCfg.AddFlagsForRollBackOptions(flags)
}

// InitFromViper initializes config from viper.Viper.
func (c *Config) InitFromViper(v *viper.Viper) {
c.Conditions = v.GetString(conditions)
c.RollOverConditions = esrolloverCfg.InitRollBackFromViper(v)
}
28 changes: 28 additions & 0 deletions pkg/es/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,34 @@ type BearerTokenAuthentication struct {
AllowFromContext bool `mapstructure:"from_context"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`
}

}

// RolloverOptions store the options for the working of auto-rollover (cron job) in the jaeger.
type RolloverOptions struct {
Manik2708 marked this conversation as resolved.
Show resolved Hide resolved
// Archive if set to true will handle archive indices also
Archive bool `mapstructure:"archive"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// The name of the ILM policy to use if ILM is active
ILMPolicyName string `mapstructure:"ilm_policy_name"`
// This stores number of seconds to wait for master node response. By default, it is set to 120
Timeout int `mapstructure:"timeout"`
// SkipDependencies if set to true will disable rollover for dependencies index
SkipDependencies bool `mapstructure:"skip_dependencies"`
// AdaptiveSampling if set to true will enable rollover for adaptive sampling index
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type LookBackOptions struct {
Manik2708 marked this conversation as resolved.
Show resolved Hide resolved
// 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"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • do we actually support weeks / months / years? Code ptr?
  • can we not simply have this as Duration type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Conditions stores the conditions on which index writing should be performed, for example: "{\"max_age\": \"2d\"}"
Conditions string `mapstructure:"conditions"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// NewClient creates a new ElasticSearch client
func NewClient(c *Configuration, logger *zap.Logger, metricsFactory metrics.Factory) (es.Client, error) {
if len(c.Servers) < 1 {
Expand Down
Loading
Loading