feat: Remove the need for external tools for managing ElasticSearch #6291

Rohanraj123 · 2024-12-02T14:08:07Z

Which problem is this PR solving?

Remove the need for external tools for managing Elasticsearch #6283

Description of the changes

I just added index-cleaner tool into jaeger binary. In furthur commits I will add other two as well. Also Documentation and tests will be added in furthur commits.

How was this change tested?

Pending

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: yarn lint and yarn test

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

codecov · 2024-12-02T14:17:13Z

Codecov Report

Attention: Patch coverage is 3.07692% with 63 lines in your changes missing coverage. Please review.

Project coverage is 95.92%. Comparing base (39e3bfb) to head (e40a5f7).
Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
plugin/storage/es/index_cleaner.go	0.00%	28 Missing ⚠️
plugin/storage/es/index_rollover.go	0.00%	19 Missing ⚠️
plugin/storage/es/es_mapping_generator.go	0.00%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6291      +/-   ##
==========================================
- Coverage   96.20%   95.92%   -0.29%     
==========================================
  Files         356      359       +3     
  Lines       20416    20519     +103     
==========================================
+ Hits        19642    19683      +41     
- Misses        585      648      +63     
+ Partials      189      188       -1

Flag	Coverage Δ
badger_v1	`8.78% <0.00%> (-0.12%)`	⬇️
badger_v2	`1.61% <0.00%> (-0.03%)`	⬇️
cassandra-4.x-v1-manual	`14.65% <0.00%> (-0.20%)`	⬇️
cassandra-4.x-v2-auto	`1.55% <0.00%> (-0.02%)`	⬇️
cassandra-4.x-v2-manual	`1.55% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v1-manual	`14.65% <0.00%> (-0.20%)`	⬇️
cassandra-5.x-v2-auto	`1.55% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v2-manual	`1.55% <0.00%> (-0.02%)`	⬇️
elasticsearch-6.x-v1	`18.36% <0.00%> (-0.25%)`	⬇️
elasticsearch-7.x-v1	`18.43% <0.00%> (-0.25%)`	⬇️
elasticsearch-8.x-v1	`18.60% <0.00%> (-0.25%)`	⬇️
elasticsearch-8.x-v2	`1.61% <0.00%> (-0.03%)`	⬇️
grpc_v1	`10.22% <0.00%> (-0.15%)`	⬇️
grpc_v2	`7.80% <0.00%> (-0.10%)`	⬇️
kafka-v1	`8.47% <0.00%> (-0.12%)`	⬇️
kafka-v2	`1.61% <0.00%> (-0.03%)`	⬇️
memory_v2	`1.61% <0.00%> (-0.03%)`	⬇️
opensearch-1.x-v1	`18.48% <0.00%> (-0.25%)`	⬇️
opensearch-2.x-v1	`18.48% <0.00%> (-0.25%)`	⬇️
opensearch-2.x-v2	`1.61% <0.00%> (-0.03%)`	⬇️
tailsampling-processor	`0.45% <0.00%> (-0.01%)`	⬇️
unittests	`94.83% <3.07%> (-0.28%)`	⬇️

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.

yurishkuro · 2024-12-03T00:43:48Z

pkg/es/config/config.go

+
+// Validation function for Cleaner struct
+func (c *Cleaner) Validate() error {
+	if c.Frequency <= 0 {


validation can be declared in the field tags

yurishkuro · 2024-12-03T00:45:18Z

pkg/es/config/config.go

+type Cleaner struct {
+	Enabled        bool          `mapstructure:"enabled"`
+	Frequency      time.Duration `mapstructure:"frequency"`
+	IncludeArchive bool          `mapstructure:"include_archive"`


in v2 archive storage is a separate instance, this property won't make sense

yurishkuro · 2024-12-03T00:45:46Z

pkg/es/config/config.go

+
+	// ---- ES-specific config ----
+	// Cleaner holds the configuration for the Elasticsearch index Cleaner.
+	Cleaner Cleaner `mapstructure:"cleaner"`


Suggested change

Cleaner Cleaner `mapstructure:"cleaner"`

IndexCleaner IndexCleaner `mapstructure:"index_cleaner"`

yurishkuro · 2024-12-03T00:47:10Z

pkg/es/config/config.go

@@ -585,3 +608,198 @@ func (c *Configuration) Validate() error {
 	_, err := govalidator.ValidateStruct(c)
 	return err
 }
+
+// Run starts the cleaner that runs periodically based on the Frequency field.
+func (cfg *Configuration) RunCleaner() {


config package is for configuration, not for business logic. This should be somewhere in plugin/storage/es

yurishkuro · 2024-12-03T00:48:04Z

pkg/es/config/config.go

+}
+
+// deleteOldIndices deletes indices older than the configured MaxSpanAge.
+func (cfg *Configuration) deleteOldIndices() error {


doesn't this logic already exist in in cmd/es-index-cleaner? Why do we need to duplicate it?

yurishkuro · 2024-12-03T00:48:48Z

cmd/jaeger/main.go

@@ -24,6 +24,8 @@ func main() {
 		command,
 	)

+	internal.StartCleaner(v)


it's enabled by config option, no need to "start" anything expicitly

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

yurishkuro · 2024-12-04T14:35:02Z

pkg/es/config/config.go

+// Rollover struct for configuring roll over
+type Rollover struct {
+	Enabled      bool          `mapstructure:"enabled" valid:"required"`
+	IndexPattern string        `mapstructure:"index_pattern" valid:"required"`


Storage already knows how indices are named, why is this arg needed?

yurishkuro · 2024-12-04T14:35:25Z

pkg/es/config/config.go

+// IndexCleaner struct for configuring index cleaning
+type IndexCleaner struct {
+	Enabled   bool          `mapstructure:"enabled"`
+	Frequency time.Duration `mapstructure:"frequency" validate:"gt=0"`


Please add comment describing the arg

yurishkuro · 2024-12-04T14:35:33Z

pkg/es/config/config.go

+type Rollover struct {
+	Enabled      bool          `mapstructure:"enabled" valid:"required"`
+	IndexPattern string        `mapstructure:"index_pattern" valid:"required"`
+	Frequency    time.Duration `mapstructure:"frequency" validate:"gt=0"`


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

Rohanraj123 · 2024-12-08T09:15:03Z

@yurishkuro Can you please review the PR once. So that i can see, whether going into right direction or not.

yurishkuro · 2024-12-08T14:28:29Z

cmd/jaeger/config-elasticsearch.yaml

@@ -36,22 +36,22 @@ extensions:
            index_prefix: "jaeger-main"
            spans:
              date_layout: "2006-01-02"
-              rollover_frequency: "day"
+              rollover_frequency: "1day"


can you create another PR (to merge before this one) that changes the RolloverFrequency field type to time.Duration ?

yurishkuro · 2024-12-08T14:33:17Z

plugin/storage/es/es_mapping_generator.go

+			}
+
+			// Run the esmapping-generator command
+			execCmd := exec.Command("go", "run", "./cmd/esmapping-generator")


How do you imagine this working in production if someone runs Jaeger as a container image? Where would they get go toolchain and Jaeger source code?

Can you guide me here little

first, I suggest you focus on one tool at a time, start with the cleaner only. The logic of the cleaner is already implemented in Go, we can call it directly as a library from the ES implementation. You can reuse all the sub-commands defined in cmd/es-cleaner, but you may have to move them out of main() into a reusable package (that would be a standalone refactoring and should be its own PR).

one more thing - I think the easiest too to start with is es-mapping, because it's purely an on-demand command run with no need to set up periodic jobs.

I was wrong in the comment above - I don't think sub-commands will be needed once the cleaner/rollover are integrated into the main binary, because the only reason those sub-commands exist today is to allow the script to be run as externally managed cron job, so it needs to do different things. But if it's run as internal periodic job, it's one combined functionality.

feat: Remove the need for external tools for managing ElasticSearch

2199d38

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

yurishkuro reviewed Dec 3, 2024

View reviewed changes

Fixed the suggestions

e2e5859

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

yurishkuro reviewed Dec 4, 2024

View reviewed changes

Rohanraj123 added 6 commits December 6, 2024 11:20

Add rollover caller from jaeger binary

02e2826

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

Add esmapping-generator to jaeger binary

aeac08a

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

Add esmapping-generator into jaeger binary

fa71243

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

Fixed the suggestions

d14a1d6

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

fixed the integration issue

00dc807

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

add frequency for rollover

6a4a0d2

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

Rohanraj123 force-pushed the issue-6283 branch from 6ca42c2 to cf7043c Compare December 6, 2024 05:52

Rohanraj123 added 2 commits December 6, 2024 11:24

Removed valid tag from enabled field

51ab560

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

Fixed unit test

e40a5f7

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

Rohanraj123 force-pushed the issue-6283 branch from cf7043c to e40a5f7 Compare December 6, 2024 05:54

Rohanraj123 marked this pull request as ready for review December 6, 2024 05:56

Rohanraj123 requested a review from a team as a code owner December 6, 2024 05:56

Rohanraj123 requested a review from pavolloffay December 6, 2024 05:56

dosubot bot added the storage/elasticsearch label Dec 6, 2024

yurishkuro reviewed Dec 8, 2024

View reviewed changes

Rohanraj123 mentioned this pull request Dec 8, 2024

feat: Add esmapping-generator into jaeger binary #6327

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Remove the need for external tools for managing ElasticSearch #6291

feat: Remove the need for external tools for managing ElasticSearch #6291

Rohanraj123 commented Dec 2, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

yurishkuro Dec 3, 2024

yurishkuro Dec 3, 2024

yurishkuro Dec 3, 2024

yurishkuro Dec 3, 2024

yurishkuro Dec 3, 2024

yurishkuro Dec 3, 2024

yurishkuro Dec 4, 2024

yurishkuro Dec 4, 2024

yurishkuro Dec 4, 2024

Rohanraj123 commented Dec 8, 2024

yurishkuro Dec 8, 2024 •

edited

Loading

Rohanraj123 Dec 8, 2024

yurishkuro Dec 8, 2024

Rohanraj123 Dec 8, 2024

Rohanraj123 Dec 8, 2024

yurishkuro Dec 8, 2024

yurishkuro Dec 8, 2024

	Cleaner Cleaner `mapstructure:"cleaner"`
	IndexCleaner IndexCleaner `mapstructure:"index_cleaner"`

feat: Remove the need for external tools for managing ElasticSearch #6291

Are you sure you want to change the base?

feat: Remove the need for external tools for managing ElasticSearch #6291

Conversation

Rohanraj123 commented Dec 2, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Dec 2, 2024 • 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

Rohanraj123 commented Dec 8, 2024

yurishkuro Dec 8, 2024 • edited Loading

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

codecov bot commented Dec 2, 2024 •

edited

Loading

yurishkuro Dec 8, 2024 •

edited

Loading