[es] Add remote read clusters option for cross-cluster querying #2874

dgrizzanti · 2021-03-11T19:16:27Z

Which problem is this PR solving?

Enables elasticsearch cross-cluster querying

Resolves #1883
Updates to PR #1892

Short description of the changes

The cross cluster query api functions by appending a pre-configured remote cluster to an index e.g. cluster_one:index_name. This change simply applies a prefix for each remote cluster and for each index. You will end up with total_indicies = indicies * remote_clusters. Also added a special case local which will not append a prefix and query the local cluster.

This works with both standard and rollover indices since it mostly comes down to a simple string manipulation.

codecov · 2021-03-11T19:22:12Z

Codecov Report

Merging #2874 (6557e5b) into master (78019b6) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2874      +/-   ##
==========================================
- Coverage   95.92%   95.89%   -0.03%     
==========================================
  Files         223      223              
  Lines        9685     9704      +19     
==========================================
+ Hits         9290     9306      +16     
- Misses        327      328       +1     
- Partials       68       70       +2

Impacted Files	Coverage Δ
plugin/storage/es/factory.go	`97.64% <100.00%> (+0.02%)`	⬆️
plugin/storage/es/options.go	`100.00% <100.00%> (ø)`
plugin/storage/es/spanstore/reader.go	`100.00% <100.00%> (ø)`
cmd/query/app/server.go	`95.62% <0.00%> (-1.46%)`	⬇️
plugin/storage/badger/spanstore/reader.go	`95.37% <0.00%> (-0.72%)`	⬇️
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78019b6...6557e5b. Read the comment docs.

albertteoh · 2021-03-12T04:48:29Z

plugin/storage/es/spanstore/reader.go

 	if archive {
 		var archiveSuffix string
 		if useReadWriteAliases {
 			archiveSuffix = archiveReadIndexSuffix
 		} else {
 			archiveSuffix = archiveIndexSuffix
 		}
-		return func(indexName, indexDateLayout string, startTime time.Time, endTime time.Time) []string {
+		return addRemoteReadClusters(func(indexName, indexDateLayout string, startTime time.Time, endTime time.Time) []string {


nit: while you're at it, please rename indexName to indexPrefix.

albertteoh · 2021-03-12T04:49:59Z

plugin/storage/es/spanstore/reader.go

 	}
 	if useReadWriteAliases {
-		return func(indices string, indexDateLayout string, startTime time.Time, endTime time.Time) []string {
+		return addRemoteReadClusters(func(indices string, indexDateLayout string, startTime time.Time, endTime time.Time) []string {


nit: also while you're at it, for consistency and readability func(indexPrefix, indexDateLayout string, ... :)

albertteoh · 2021-03-12T04:54:47Z

plugin/storage/es/spanstore/reader.go

+// Add a remote cluster prefix for each cluster and for each index and add it to the list of original indices
+func addRemoteReadClusters(fn timeRangeIndexFn, remoteReadClusters []string) timeRangeIndexFn {
+	return func(indexName string, indexDateLayout string, startTime time.Time, endTime time.Time) []string {
+		jaegerIndices := fn(indexName, indexDateLayout, startTime, endTime)


nit: we could short-circuit and reduce nesting with:

if len(remoteReadCluster) == 0 { return jaegerIndices }

plugin/storage/es/spanstore/reader_test.go

albertteoh · 2021-03-12T12:09:39Z

plugin/storage/es/spanstore/reader.go

+// Elasticsearch cross cluster query api example: GET /twitter,cluster_one:twitter,cluster_two:twitter/_search
+// Add a remote cluster prefix for each cluster and for each index and add it to the list of original indices


nit: Please end comment sentences with a full-stop.

Same as above though I understand this convention is not followed in the adjacent functions below it.

dgrizzanti · 2021-03-12T14:15:18Z

@albertteoh thanks for the comments and suggestions. I think I covered all of them if you can take a look again. Thanks!

albertteoh · 2021-03-12T23:38:19Z

@albertteoh thanks for the comments and suggestions. I think I covered all of them if you can take a look again. Thanks!

One more :) Also, it seems another commit was pulled into this PR: 5105237.

albertteoh · 2021-03-12T23:35:11Z

plugin/storage/integration/integration.go

@@ -36,7 +36,7 @@ import (
 )

 const (
-	iterations = 30
+	iterations = 100


Were these changes brought in from #2873?

I did rebase so may be they got pulled in by accident? I flip it back so it doesn't show up in this PR.

Yup, I had more luck with git merge and did recall having similar problems when using git rebase. Thanks 🙏

…ter querying Co-authored-by: allen13 <[email protected]> Signed-off-by: David Grizzanti <[email protected]>

* Increasing test timeout to allow for trace storage settle Signed-off-by: Jacob Goldsworthy <[email protected]> * Updating max storage test wait time to 10 seconds Signed-off-by: Jacob Goldsworthy <[email protected]> Signed-off-by: David Grizzanti <[email protected]>

…when remoteReadClusters is emtpy Signed-off-by: David Grizzanti <[email protected]>

…ReadWriteAliases: true Signed-off-by: David Grizzanti <[email protected]>

Signed-off-by: David Grizzanti <[email protected]>

* Increasing test timeout to allow for trace storage settle Signed-off-by: Jacob Goldsworthy <[email protected]> * Updating max storage test wait time to 10 seconds Signed-off-by: Jacob Goldsworthy <[email protected]> Signed-off-by: David Grizzanti <[email protected]>

Signed-off-by: David Grizzanti <[email protected]>

albertteoh · 2021-03-14T07:56:30Z

pkg/es/config/config.go

@@ -193,6 +195,9 @@ func (c *Configuration) NewClient(logger *zap.Logger, metricsFactory metrics.Fac

 // ApplyDefaults copies settings from source unless its own value is non-zero.
 func (c *Configuration) ApplyDefaults(source *Configuration) {
+	if len(c.RemoteReadClusters) == 0 {
+		c.RemoteReadClusters = source.RemoteReadClusters


Will this line ever be hit? I believe c.RemoteReadClusters defaults to []string{""} because strings.Split() should result in a non-empty slice overwriting the initial empty slice.

Thanks @albertteoh - I didn't realize this nuance with strings.Split() but after looking at the docs, this makes sense. I didn't see a great (easy) way around this since the cli takes in strings, so I updated options.go to check the length and only split when necessary (otherwise default to []string{}). I added 2 tests cases around options as well to cover this.

Thanks for adding the test cases to cover the empty string. I think there's also the following case: --es.remote-read-clusters=cluster_one,,cluster_two, where the second item is empty but I think we can consider this a user error.

albertteoh · 2021-03-14T08:01:46Z

plugin/storage/es/spanstore/reader.go

 		}
+
+		for _, jaegerIndex := range jaegerIndices {
+			for _, remoteCluster := range remoteReadClusters {


Related to the earlier comment, if remoteReadClusters defaults to []string{""}, the returned indices count would be doubled with the new indices looking like :<index-name>. Is that expected?

This should be avoided now with the changes to plugin/storage/es/options.go

…sters are provided Signed-off-by: David Grizzanti <[email protected]>

albertteoh · 2021-03-15T05:29:56Z

plugin/storage/es/options_test.go

+	opts.InitFromViper(v)
+
+	primary := opts.GetPrimary()
+	assert.Equal(t, []string{}, primary.RemoteReadClusters)


I think the following would be more expressive/readable: assert.Empty(t, primary.RemoteReadClusters)

albertteoh · 2021-03-15T05:36:06Z

pkg/es/config/config.go

@@ -193,6 +195,9 @@ func (c *Configuration) NewClient(logger *zap.Logger, metricsFactory metrics.Fac

 // ApplyDefaults copies settings from source unless its own value is non-zero.
 func (c *Configuration) ApplyDefaults(source *Configuration) {
+	if len(c.RemoteReadClusters) == 0 {
+		c.RemoteReadClusters = source.RemoteReadClusters


Thanks for adding the test cases to cover the empty string. I think there's also the following case: --es.remote-read-clusters=cluster_one,,cluster_two, where the second item is empty but I think we can consider this a user error.

albertteoh

lgtm, thanks @dgrizzanti.

yurishkuro · 2021-03-16T03:25:09Z

Thank you!

dgrizzanti requested a review from a team as a code owner March 11, 2021 19:16

dgrizzanti requested a review from objectiser March 11, 2021 19:16

dgrizzanti force-pushed the elastic-cross-cluster-search branch from bf46138 to 0e38cfa Compare March 11, 2021 19:36

albertteoh reviewed Mar 12, 2021

View reviewed changes

dgrizzanti force-pushed the elastic-cross-cluster-search branch from 27af427 to a84a365 Compare March 12, 2021 13:44

albertteoh reviewed Mar 12, 2021

View reviewed changes

Add remote read clusters option to elasticsearch to enable cross-clus…

7f9b50e

…ter querying Co-authored-by: allen13 <[email protected]> Signed-off-by: David Grizzanti <[email protected]>

dgrizzanti force-pushed the elastic-cross-cluster-search branch from 624881a to 386dd42 Compare March 13, 2021 21:44

LostLaser and others added 7 commits March 13, 2021 16:52

Rename indexName to indexPrefix; short circuit addRemoteReadClusters …

51ee793

…when remoteReadClusters is emtpy Signed-off-by: David Grizzanti <[email protected]>

Add more test cases for remoteReadClusters with Archive: true and Use…

5ecffd3

…ReadWriteAliases: true Signed-off-by: David Grizzanti <[email protected]>

Removing unnecessary changes from this PR

95c3bf3

Signed-off-by: David Grizzanti <[email protected]>

Update variable name for consistency

15a0ee6

Signed-off-by: David Grizzanti <[email protected]>

Fix index prefix name

aab1458

Signed-off-by: David Grizzanti <[email protected]>

dgrizzanti force-pushed the elastic-cross-cluster-search branch from 6e69076 to aab1458 Compare March 13, 2021 21:53

Merge branch 'master' into elastic-cross-cluster-search

29365cb

albertteoh reviewed Mar 14, 2021

View reviewed changes

dgrizzanti force-pushed the elastic-cross-cluster-search branch from 3edd6b2 to 16c950a Compare March 14, 2021 17:13

Prevent remoteReadClusters from being []string{""} when no remote clu…

6557e5b

…sters are provided Signed-off-by: David Grizzanti <[email protected]>

dgrizzanti force-pushed the elastic-cross-cluster-search branch from 16c950a to 6557e5b Compare March 14, 2021 18:01

albertteoh reviewed Mar 15, 2021

View reviewed changes

albertteoh approved these changes Mar 15, 2021

View reviewed changes

yurishkuro changed the title ~~Add remote read clusters option to elasticsearch to enable cross-cluster querying~~ [es] Add remote read clusters option for cross-cluster querying Mar 16, 2021

yurishkuro merged commit 915d3af into jaegertracing:master Mar 16, 2021

This was referenced Mar 16, 2021

Bump jaeger from 72b9a06 to 915d3af querycap/jaeger#36

Closed

Bump jaeger from 72b9a06 to 40e96f9 querycap/jaeger#37

Closed

WalkerWang731 mentioned this pull request Apr 13, 2021

[abandoned][es] Add index rollover mode that can choose day and hour #2935

Closed

jpkrohling added this to the Release 1.23.0 milestone Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[es] Add remote read clusters option for cross-cluster querying #2874

[es] Add remote read clusters option for cross-cluster querying #2874

dgrizzanti commented Mar 11, 2021

codecov bot commented Mar 11, 2021 •

edited

Loading

albertteoh Mar 12, 2021

albertteoh Mar 12, 2021

albertteoh Mar 12, 2021

albertteoh Mar 12, 2021

dgrizzanti commented Mar 12, 2021

albertteoh commented Mar 12, 2021

albertteoh Mar 12, 2021

dgrizzanti Mar 13, 2021

albertteoh Mar 14, 2021

albertteoh Mar 14, 2021

dgrizzanti Mar 14, 2021

albertteoh Mar 15, 2021

albertteoh Mar 14, 2021

dgrizzanti Mar 14, 2021

albertteoh Mar 15, 2021

albertteoh Mar 15, 2021

albertteoh left a comment

yurishkuro commented Mar 16, 2021

		// Elasticsearch cross cluster query api example: GET /twitter,cluster_one:twitter,cluster_two:twitter/_search
		// Add a remote cluster prefix for each cluster and for each index and add it to the list of original indices

[es] Add remote read clusters option for cross-cluster querying #2874

[es] Add remote read clusters option for cross-cluster querying #2874

Conversation

dgrizzanti commented Mar 11, 2021

Which problem is this PR solving?

Short description of the changes

codecov bot commented Mar 11, 2021 • 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

dgrizzanti commented Mar 12, 2021

albertteoh commented Mar 12, 2021

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

albertteoh left a comment

Choose a reason for hiding this comment

yurishkuro commented Mar 16, 2021

codecov bot commented Mar 11, 2021 •

edited

Loading