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

WalkerWang731 · 2021-04-28T02:21:45Z

Which problem is this PR solving?

Short description of the changes

Add two parameters index-rollover-frequency-spans and index-rollover-frequency-services, default is day mode, will not affect existing usage
Minimize changes to ensure compatibility with previous code

Parameter config of `collector` `ingester` `query` (SPAN_STORAGE_TYPE=elasticsearch)

--es-archive.index-rollover-frequency-services string   Rotates jaeger-service indices over the given period. For example "day" creates "jaeger-service-yyyy-MM-dd" every day after UTC 12AM. Valid options: [hour, day]. Jaeger additionally supports manual and automated (via ILM) index management. Reference: https://www.jaegertracing.io/docs/deployment/#elasticsearch-rollover. (default "day")
--es-archive.index-rollover-frequency-spans string      Rotates jaeger-span indices over the given period. For example "day" creates "jaeger-span-yyyy-MM-dd" every day after UTC 12AM. Valid options: [hour, day]. Jaeger additionally supports manual and automated (via ILM) index management. Reference: https://www.jaegertracing.io/docs/deployment/#elasticsearch-rollover. (default "day")
--es.index-rollover-frequency-services string           Rotates jaeger-service indices over the given period. For example "day" creates "jaeger-service-yyyy-MM-dd" every day after UTC 12AM. Valid options: [hour, day]. Jaeger additionally supports manual and automated (via ILM) index management. Reference: https://www.jaegertracing.io/docs/deployment/#elasticsearch-rollover. (default "day")
--es.index-rollover-frequency-spans string              Rotates jaeger-span indices over the given period. For example "day" creates "jaeger-span-yyyy-MM-dd" every day after UTC 12AM. Valid options: [hour, day]. Jaeger additionally supports manual and automated (via ILM) index management. Reference: https://www.jaegertracing.io/docs/deployment/#elasticsearch-rollover. (default "day")

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

codecov · 2021-04-28T02:28:33Z

Codecov Report

Merging #2965 (e8e5388) into master (d43af83) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2965      +/-   ##
==========================================
+ Coverage   95.95%   95.97%   +0.02%     
==========================================
  Files         224      224              
  Lines        9747     9773      +26     
==========================================
+ Hits         9353     9380      +27     
+ Misses        325      324       -1     
  Partials       69       69

Impacted Files	Coverage Δ
plugin/storage/es/factory.go	`97.75% <100.00%> (+0.10%)`	⬆️
plugin/storage/es/options.go	`100.00% <100.00%> (ø)`
plugin/storage/es/spanstore/reader.go	`100.00% <100.00%> (ø)`
plugin/storage/es/spanstore/writer.go	`100.00% <100.00%> (ø)`
plugin/storage/badger/spanstore/reader.go	`95.37% <0.00%> (-0.72%)`	⬇️
plugin/storage/integration/integration.go	`77.34% <0.00%> (-0.56%)`	⬇️
cmd/query/app/static_handler.go	`96.77% <0.00%> (+1.61%)`	⬆️
pkg/config/tlscfg/cert_watcher.go	`94.80% <0.00%> (+2.59%)`	⬆️

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 d43af83...e8e5388. Read the comment docs.

WalkerWang731 · 2021-04-28T02:34:36Z

Hi @albertteoh
Sorry for the trouble with you. I have recreated a new PR that repair the signoff issue (here) and repair CI issue (here), let us continue on here

…orage

albertteoh · 2021-04-28T11:27:05Z

plugin/storage/es/spanstore/reader.go

@@ -199,9 +200,16 @@ func timeRangeIndices(indexName, indexDateLayout string, startTime time.Time, en
 	var indices []string
 	firstIndex := indexWithDate(indexName, indexDateLayout, startTime)
 	currentIndex := indexWithDate(indexName, indexDateLayout, endTime)
+
+	reduce := -24 * time.Hour
+	if strings.HasSuffix(indexDateLayout, "15") {


It's a shame time.stdHour isn't exported.

A cleaner way is to pass freq directly as parameter, not infer it back from format string

Yeah, I also thought previously, but if pass freq directly then will also judge this variable too(if freq == "day" or move this judgment to ClientBuilder of config.go ), basically it same as currently.

And another side, other functions will be become more complicated include NewSpanReader SpanReaderParams SpanReader addRemoteReadClusters timeRangeIndices createSpanWriter and reader relevant factory functions and all of unit test functions. and change the ClientBuilder of config.go.

Actually, I always thought this commit should be small and should not change more code, but if we need, I can do it

fair enough. As Albert said, it would be good to at least use a named constant shared between config and span reader

ok, I see, now that we decide to add the new constant, I want to separate rollover jaeger-span and jaeger-service incidentally. How about?
Because the reason can refer here
I will add two params index-rollover-span-frequency and index-rollover-span-frequency, do you have any suggestions?

Sounds good. I would suggest these names index-rollover-frequency-spans and index-rollover-frequency-services

yurishkuro · 2021-04-28T14:31:46Z

plugin/storage/es/options.go

+		nsConfig.namespace+suffixIndexRolloverFrequency,
+		defaultIndexRolloverFrequency,
+		"Rotates Jaeger indices over the given period. For example \"day\" creates \"jaeger-span-yyyy-MM-dd\" every day after UTC 12AM. Valid options: [hour, day]. "+
+			"Jaeger additionally supports manual and automated (via ILM) index management. Reference: https://www.jaegertracing.io/docs/deployment/#elasticsearch-rollover.")


Didn't we decide to change this msg?

@WalkerWang731 I believe we suggested to reword this to (@yurishkuro please correct if I'm wrong):

This does not delete old indices. For details on complete index management solutions supported by Jaeger, refer to: https://www.jaegertracing.io/docs/deployment/#elasticsearch-rollover

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

…hub.com:WalkerWang731/jaeger into walkerwang731/add_index_rollover_for_es_storage Signed-off-by: WalkerWang731 <[email protected]>

Pull request has been modified.

…defaultIndexRolloverFrequency Signed-off-by: WalkerWang731 <[email protected]>

WalkerWang731 · 2021-05-05T07:12:28Z

Hi @yurishkuro @albertteoh,
I had updated and added two params index-rollover-frequency-spans and index-rollover-frequency-services

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

…orage

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

…orage

Signed-off-by: Albert Teoh <[email protected]>

…hub.com:WalkerWang731/jaeger into walkerwang731/add_index_rollover_for_es_storage

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

…orage

albertteoh · 2021-05-17T13:31:11Z

Thanks @WalkerWang731!

add index rollover for ES storage

37751e6

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

WalkerWang731 requested a review from a team as a code owner April 28, 2021 02:21

WalkerWang731 requested a review from pavolloffay April 28, 2021 02:21

WalkerWang731 mentioned this pull request Apr 28, 2021

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

Closed

Merge branch 'master' into walkerwang731/add_index_rollover_for_es_st…

c704add

…orage

albertteoh previously approved these changes Apr 28, 2021

View reviewed changes

yurishkuro reviewed Apr 28, 2021

View reviewed changes

WalkerWang731 added 2 commits April 30, 2021 00:30

update that separate rollover of spans and services for the es index

b39325b

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

Merge branch 'walkerwang731/add_index_rollover_for_es_storage' of git…

632700b

…hub.com:WalkerWang731/jaeger into walkerwang731/add_index_rollover_for_es_storage Signed-off-by: WalkerWang731 <[email protected]>

fix the GetOperation issue and change IndexDateLayoutDependencies to …

9788c45

…defaultIndexRolloverFrequency Signed-off-by: WalkerWang731 <[email protected]>

WalkerWang731 and others added 8 commits May 13, 2021 15:31

update explain for the es.index-rollover-frequency

eacd0ad

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

Merge branch 'master' into walkerwang731/add_index_rollover_for_es_st…

2d6733b

…orage

make fmt

915666d

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

Merge branch 'master' into walkerwang731/add_index_rollover_for_es_st…

e9f8cbb

…orage

Fix minor formatting issues in table tests

ba7b82e

Signed-off-by: Albert Teoh <[email protected]>

Merge branch 'walkerwang731/add_index_rollover_for_es_storage' of git…

0e54999

…hub.com:WalkerWang731/jaeger into walkerwang731/add_index_rollover_for_es_storage

missed nits in test

ff27c96

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

Merge branch 'master' into walkerwang731/add_index_rollover_for_es_st…

e8e5388

…orage

albertteoh approved these changes May 17, 2021

View reviewed changes

albertteoh merged commit 19cbe35 into jaegertracing:master May 17, 2021

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 index rollover mode that can choose day and hour #2965

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

WalkerWang731 commented Apr 28, 2021 •

edited

Loading

codecov bot commented Apr 28, 2021 •

edited

Loading

WalkerWang731 commented Apr 28, 2021 •

edited

Loading

albertteoh Apr 28, 2021

yurishkuro Apr 28, 2021

WalkerWang731 Apr 28, 2021

yurishkuro Apr 28, 2021 •

edited

Loading

WalkerWang731 Apr 28, 2021 •

edited

Loading

yurishkuro Apr 28, 2021

yurishkuro Apr 28, 2021

albertteoh May 6, 2021

WalkerWang731 commented May 5, 2021

albertteoh commented May 17, 2021

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

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

Conversation

WalkerWang731 commented Apr 28, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

Parameter config of collector ingester query (SPAN_STORAGE_TYPE=elasticsearch)

codecov bot commented Apr 28, 2021 • edited Loading

Codecov Report

WalkerWang731 commented Apr 28, 2021 • edited Loading

albertteoh Apr 28, 2021

Choose a reason for hiding this comment

yurishkuro Apr 28, 2021

Choose a reason for hiding this comment

WalkerWang731 Apr 28, 2021

Choose a reason for hiding this comment

yurishkuro Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

WalkerWang731 Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

yurishkuro Apr 28, 2021

Choose a reason for hiding this comment

yurishkuro Apr 28, 2021

Choose a reason for hiding this comment

albertteoh May 6, 2021

Choose a reason for hiding this comment

WalkerWang731 commented May 5, 2021

albertteoh commented May 17, 2021

WalkerWang731 commented Apr 28, 2021 •

edited

Loading

Parameter config of `collector` `ingester` `query` (SPAN_STORAGE_TYPE=elasticsearch)

codecov bot commented Apr 28, 2021 •

edited

Loading

WalkerWang731 commented Apr 28, 2021 •

edited

Loading

yurishkuro Apr 28, 2021 •

edited

Loading

WalkerWang731 Apr 28, 2021 •

edited

Loading