[jaeger-v2] Refactor ElasticSearch/OpenSearch configurations to have more logical groupings #6090

mahadzaryab1 · 2024-10-15T03:05:48Z

Which problem is this PR solving?

Resolves [jaeger-v2] Refactor ElasticSearch/OpenSearch Storage Configurations To Align With OTEL #6059

Description of the changes

Added more groupings for the following sub-configurations in the ElasticSearch/OpenSearch configurations
- Sniffing
- Authentication
- Bulk Processing
The migration guide from v1 to v2 can be viewed here.

How was this change tested?

CI

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: Mahad Zaryab <mahadzaryab1@gmail.com>

codecov · 2024-10-15T03:13:47Z

Codecov Report

Attention: Patch coverage is 98.78049% with 1 line in your changes missing coverage. Please review.

Project coverage is 96.46%. Comparing base (32112eb) to head (dc17b76).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/es/config/config.go	97.05%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6090   +/-   ##
=======================================
  Coverage   96.46%   96.46%           
=======================================
  Files         352      352           
  Lines       19953    19961    +8     
=======================================
+ Hits        19247    19255    +8     
  Misses        522      522           
  Partials      184      184

Flag	Coverage Δ
badger_v1	`8.44% <0.00%> (-0.01%)`	⬇️
badger_v2	`1.71% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v1	`14.59% <0.00%> (-0.02%)`	⬇️
cassandra-4.x-v2	`1.65% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v1	`14.59% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v2	`1.65% <0.00%> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`18.68% <65.85%> (+0.05%)`	⬆️
elasticsearch-7.x-v1	`18.77% <65.85%> (+0.05%)`	⬆️
elasticsearch-8.x-v1	`18.94% <69.51%> (+0.05%)`	⬆️
elasticsearch-8.x-v2	`1.70% <0.00%> (-0.01%)`	⬇️
grpc_v1	`8.80% <0.00%> (-0.01%)`	⬇️
grpc_v2	`6.73% <0.00%> (+<0.01%)`	⬆️
kafka-v1	`9.01% <0.00%> (-0.01%)`	⬇️
kafka-v2	`1.71% <0.00%> (-0.01%)`	⬇️
memory_v2	`1.71% <0.00%> (-0.01%)`	⬇️
opensearch-1.x-v1	`18.82% <65.85%> (+0.06%)`	⬆️
opensearch-2.x-v1	`18.82% <65.85%> (+0.06%)`	⬆️
opensearch-2.x-v2	`1.71% <0.00%> (-0.01%)`	⬇️
tailsampling-processor	`0.48% <0.00%> (-0.01%)`	⬇️
unittests	`95.38% <98.78%> (+<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: Mahad Zaryab <mahadzaryab1@gmail.com>

plugin/storage/es/options.go

pkg/es/config/config.go

yurishkuro · 2024-10-16T13:07:31Z

pkg/es/config/config.go

+	Shards int64 `mapstructure:"shards"`
+	// Replicas is the number of replicas per index in Elasticsearch.
+	Replicas int64 `mapstructure:"replicas"`
+	// RolloverFrequency rotates indices over the given period. For example, "day" creates


Thanks for adding docs. Is this behavior unconditional or only in some cases, like when useILM is false?

@yurishkuro I believe its unconditional. it seems to be used in the reader (https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/es/spanstore/reader.go#L275)

no, it's not unconditional. See reader.go:151 - this param is not used when archive || useReadWriteAliases. Also, the doc string is not even talking about using the param at read time, it says "rotates" - and I think that's definitely not unconditional.

@yurishkuro Is this rotation handled by ILM and this field here is just used for reading? https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/es/factory.go#L273-L282. I don't see this field being used for any writing.

Yes, I think you were right. The index rotation is always external to Jaeger, either via a rollover script or via ILM. And the reader only needs this setting to know how to translate a [from, to] interval into distinct index names (by doing t=t.Add(interval))

But given that, the doc string is wrong as it talks about creating rollover indices.

pkg/es/config/config.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

pkg/es/config/config.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

pkg/es/config/config.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro · 2024-10-18T19:22:35Z

pkg/es/config/config.go

-	options.DiscoverNodesOnStart = c.Sniffer
+	options.Username = c.Authentication.BasicAuthentication.Username
+	options.Password = c.Authentication.BasicAuthentication.Password
+	options.DiscoverNodesOnStart = c.Sniffing.Enabled


so for v8 only the enabled is used, not the UseHTTPS? Is it because v8 client always uses https?

@yurishkuro yeah, that seems to be the case. The following is used when constructing the client.

// addrFromCloudID extracts the Elasticsearch URL from CloudID. // See: https://www.elastic.co/guide/en/cloud/current/ec-cloud-id.html func addrFromCloudID(input string) (string, error) { var scheme = "https://" ... }

It would be good to have a comment on the field mentioning that.

@yurishkuro #6103

yurishkuro · 2024-10-18T19:34:31Z

pkg/es/config/config.go

 			logger.Warn("Token file and token propagation are both enabled, token from file won't be used")
 		}
-		tokenFromFile, err := loadTokenFromFile(c.TokenFilePath)
+		tokenFromFile, err := loadTokenFromFile(c.Authentication.BearerTokenAuthentication.FilePath)


weird that despite the PR title #4342 there is no "reloading" of the file, only at init time (whereas password does get reloaded). But since nobody complained ...

I don't see much documentation on the website about how this token is supposed to work, specifically any pointers to the documentation for the actual storage (e.g. ES or hosted ES). Might have been some references in the original issues but hard to find them.

I guess this one: https://www.elastic.co/guide/en/elasticsearch/reference/current/token-authentication-services.html

## Which problem is this PR solving? - Part of #6059 ## Description of the changes - Added some remaining documentation to the ElasticSearch configuration that was left over from #6090 ## How was this change tested? - CI ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

…rtracing#6103) ## Which problem is this PR solving? - Part of jaegertracing#6059 ## Description of the changes - Added some remaining documentation to the ElasticSearch configuration that was left over from jaegertracing#6090 ## How was this change tested? - CI ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

…more logical groupings (jaegertracing#6090) ## Which problem is this PR solving? - Resolves jaegertracing#6059 ## Description of the changes - Added more groupings for the following sub-configurations in the ElasticSearch/OpenSearch configurations - Sniffing - Authentication - Bulk Processing - The migration guide from v1 to v2 can be viewed [here](https://docs.google.com/document/d/1rabu8zvjoZeHx-HNqvK5kjsujMEBC7DkR2MYeOvB6HI/edit?tab=t.0#heading=h.abjgb642qlsg). ## How was this change tested? - CI ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

…rtracing#6103) ## Which problem is this PR solving? - Part of jaegertracing#6059 ## Description of the changes - Added some remaining documentation to the ElasticSearch configuration that was left over from jaegertracing#6090 ## How was this change tested? - CI ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

…to have more logical groupings (jaegertracing#6090)" This reverts commit fe700fe.

mahadzaryab1 added 4 commits October 14, 2024 20:59

Add Separate Config Struct For Sniffing

2a47ab5

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Group Bulk Processing Configurations Together

b8642e7

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Group Authentication Together

48e652d

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add Missing Mapstructure Tags

d190973

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added 2 commits October 15, 2024 20:12

Add Documentation To Config Structs

dd330c8

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Use Configured Fields For Initialization

9b2f124

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 commented Oct 16, 2024

View reviewed changes

plugin/storage/es/options.go Show resolved Hide resolved

mahadzaryab1 changed the title ~~[WIP][jaeger-v2] Refactor ElasticSearch/OpenSearch configurations to have more logical groupings~~ [jaeger-v2] Refactor ElasticSearch/OpenSearch configurations to have more logical groupings Oct 16, 2024

mahadzaryab1 marked this pull request as ready for review October 16, 2024 00:17

mahadzaryab1 requested a review from a team as a code owner October 16, 2024 00:17

mahadzaryab1 requested a review from pavolloffay October 16, 2024 00:17

dosubot bot added storage/elasticsearch v2 labels Oct 16, 2024

mahadzaryab1 mentioned this pull request Oct 16, 2024

[jaeger-v2] Refactor ElasticSearch/OpenSearch Storage Configurations To Align With OTEL #6059

Closed

6 tasks

mahadzaryab1 added 2 commits October 15, 2024 20:18

Merge branch 'main' into consolidate-es

4aff8fc

Merge branch 'main' into consolidate-es

c439adf

yurishkuro reviewed Oct 16, 2024

View reviewed changes

yurishkuro added the changelog:exprimental Change to an experimental part of the code label Oct 16, 2024

Address Feedback From PR Review

fa0560b

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 requested a review from yurishkuro October 16, 2024 22:37

yurishkuro reviewed Oct 17, 2024

View reviewed changes

pkg/es/config/config.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 17, 2024

View reviewed changes

pkg/es/config/config.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 17, 2024

View reviewed changes

pkg/es/config/config.go Show resolved Hide resolved

mahadzaryab1 and others added 3 commits October 17, 2024 21:12

Create Separate Grouping For Bearer Authentication

7300c33

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Group Fields Together For Readability And Maintainability

f647de0

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Merge branch 'main' into consolidate-es

d55411b

mahadzaryab1 requested a review from yurishkuro October 18, 2024 01:42

yurishkuro reviewed Oct 18, 2024

View reviewed changes

Address Feedback From PR Review

dc17b76

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 requested a review from yurishkuro October 18, 2024 03:50

yurishkuro approved these changes Oct 18, 2024

View reviewed changes

yurishkuro merged commit fe700fe into jaegertracing:main Oct 18, 2024
49 checks passed

mahadzaryab1 mentioned this pull request Oct 19, 2024

[chore][docs] Add documentation to ElasticSearch configuration #6103

Merged

4 tasks

mahadzaryab1 deleted the consolidate-es branch October 31, 2024 22:35

yurishkuro added a commit to yurishkuro/jaeger that referenced this pull request Nov 2, 2024

Revert "[jaeger-v2] Refactor ElasticSearch/OpenSearch configurations …

39c3bab

…to have more logical groupings (jaegertracing#6090)" This reverts commit fe700fe.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jaeger-v2] Refactor ElasticSearch/OpenSearch configurations to have more logical groupings #6090

[jaeger-v2] Refactor ElasticSearch/OpenSearch configurations to have more logical groupings #6090

mahadzaryab1 commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

yurishkuro Oct 16, 2024

mahadzaryab1 Oct 16, 2024

yurishkuro Oct 17, 2024

mahadzaryab1 Oct 18, 2024

yurishkuro Oct 18, 2024

yurishkuro Oct 18, 2024

yurishkuro Oct 18, 2024

mahadzaryab1 Oct 19, 2024

yurishkuro Oct 19, 2024

mahadzaryab1 Oct 19, 2024

yurishkuro Oct 18, 2024

yurishkuro Oct 18, 2024

[jaeger-v2] Refactor ElasticSearch/OpenSearch configurations to have more logical groupings #6090

[jaeger-v2] Refactor ElasticSearch/OpenSearch configurations to have more logical groupings #6090

Conversation

mahadzaryab1 commented Oct 15, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Oct 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahadzaryab1 commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading