Support configurable date separator for Elasticsearch index names #2637

sniperking1234 · 2020-11-20T08:16:59Z

Which problem is this PR solving?

Fixes elasticsearch storage can not customize the date format #1489
Jaeger can customize the es date format.

Short description of the changes

User can configure the date separator to change the date format.
Default date formate is 2006-01-02, it won't affect the old version.
If the separator is none, there is no separation in the time format. Like 20201120.

I don't know if this change is appropriate,.If so, I will modify the spark dependency and operator projects.

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

codecov · 2020-11-20T08:32:16Z

Codecov Report

Merging #2637 (21a34c0) into master (ea127a1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2637   +/-   ##
=======================================
  Coverage   95.54%   95.54%           
=======================================
  Files         214      214           
  Lines        9535     9546   +11     
=======================================
+ Hits         9110     9121   +11     
  Misses        346      346           
  Partials       79       79

Impacted Files	Coverage Δ
plugin/storage/es/dependencystore/storage.go	`91.11% <100.00%> (+0.20%)`	⬆️
plugin/storage/es/factory.go	`100.00% <100.00%> (ø)`
plugin/storage/es/options.go	`100.00% <100.00%> (ø)`
plugin/storage/es/spanstore/index_utils.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/integration/integration.go	`77.34% <0.00%> (-0.56%)`	⬇️
cmd/collector/app/server/zipkin.go	`76.92% <0.00%> (+3.84%)`	⬆️

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 ea127a1...21a34c0. Read the comment docs.

albertteoh

Thanks for putting this together! I have a few comments about simplifying the flag default value and how we define the empty separator. Let me know what you think.

plugin/storage/es/options.go

albertteoh · 2020-11-20T12:10:38Z

plugin/storage/es/esCleaner.py

+    date_regex = "\d{4}-\d{2}-\d{2}"
+    time_string = "%Y-%m-%d"
+    if separator != "":
+        if separator == "none":
+            date_regex = "\d{4}\d{2}\d{2}"
+            time_string = "%Y%m%d"
+        else:
+            date_regex = "\d{4}" + separator + "\d{2}" + separator + "\d{2}"
+            time_string = "%Y" + separator + "%m" + separator + "%d"


If we follow the suggestion above, then this could reduce down to:

date_regex = "\d{4}" + separator + "\d{2}" + separator + "\d{2}" time_string = "%Y" + separator + "%m" + separator + "%d"

Otherwise, could simplify the original logic to:

if not separator: separator = "-" elif separator == "none": separator = "" date_regex = "\d{4}" + separator + "\d{2}" + separator + "\d{2}" time_string = "%Y" + separator + "%m" + separator + "%d"

albertteoh · 2020-11-20T12:35:05Z

plugin/storage/es/options_test.go

+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			dateLayout := initDateLayout(tc.separator)
+			assert.Equal(t, dateLayout, tc.wantLayout)


Expectation comes first, i.e. assert.Equal(t, tc.wantLayout, dateLayout)

albertteoh · 2020-11-20T12:55:56Z

plugin/storage/es/options.go

@@ -210,6 +213,10 @@ func addFlags(flagSet *flag.FlagSet, nsConfig *namespaceConfig) {
 		nsConfig.namespace+suffixIndexPrefix,
 		nsConfig.IndexPrefix,
 		"Optional prefix of Jaeger indices. For example \"production\" creates \"production-jaeger-*\".")
+	flagSet.String(
+		nsConfig.namespace+suffixIndexDateSeparator,
+		"",


Could we set the default to what we want which is "-"?

albertteoh · 2020-11-20T12:56:40Z

plugin/storage/es/esCleaner.py

@@ -33,14 +34,15 @@ def main():
    prefix = os.getenv("INDEX_PREFIX", '')
    if prefix != '':
        prefix += '-'
+    separator = os.getenv("INDEX_DATE_SEPARATOR", '')


As in other comments, I think we should default this to the default we want which is '-'?

If users want the empty separator, they can export INDEX_DATE_SEPARATOR='', which seems more intuitive to me.

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

sniperking1234 · 2020-11-23T07:42:02Z

@albertteoh I made some changes, please review it again. Thank you!

albertteoh

Looks good to me, @sniperking1234 thanks! I've added a few minor comments.

plugin/storage/es/options.go

plugin/storage/es/options_test.go

albertteoh · 2020-11-23T09:50:54Z

plugin/storage/es/options_test.go

@@ -55,10 +55,12 @@ func TestOptionsWithFlags(t *testing.T) {
 		"--es.max-span-age=48h",
 		"--es.num-shards=20",
 		"--es.num-replicas=10",
+		"--es.index-date-separator=-",


I prefer testing "--es.index-date-separator=" here because it's a unique case of passing in an empty value along with other flags. The "--es.aux.index-date-separator=." below will test for a value passed in.

TestIndexDateSeparator will then test the different possible separator inputs.

albertteoh · 2020-11-23T09:59:43Z

plugin/storage/es/options_test.go

+		flags          []string
+		wantDateLayout string
+	}{
+		{"not defined", []string{}, "2006-01-02"},


I would also add tests for the following:

/. Example: 2006/01/02

''. Example: 2006''01''02. I know it's odd to add such a test but I think it communicates two two things to developers:

Separators are currently not limited to single chars, unless if we enforce single char separators in this PR.

Attempting to pass in an empty quoted string will actually use the quotes literally as separators.

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

albertteoh · 2020-11-23T13:35:36Z

plugin/storage/es/options_test.go

+		{"dot separator", []string{"--es.index-date-separator=."}, "2006.01.02"},
+		{"crossbar separator", []string{"--es.index-date-separator=-"}, "2006-01-02"},
+		{"backslash separator", []string{"--es.index-date-separator=/"}, "2006/01/02"},
+		{"multiple characters separator", []string{"--es.index-date-separator=' '"}, "2006' '01' '02"},


I actually meant empty string '' to make it clear that empty quoted string is not treated as an empty separator :)

Updated. Please check if the description of the test is appropriate.

plugin/storage/es/options_test.go

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

albertteoh

LGTM, @sniperking1234, thanks!

sniperking1234 · 2020-11-24T01:20:56Z

CI failed in TestReload unit test, but passed in my local test. How to fix it?

yurishkuro · 2020-11-24T01:25:00Z

@sniperking1234 it looks like the test is flaky, #2644.

sniperking1234 · 2020-12-02T02:48:51Z

@yurishkuro Is there any way to rerun the test or fix this error? I think I'm stuck in these tests. 😂

yurishkuro · 2020-12-02T03:54:24Z

plugin/storage/es/options.go

+	flagSet.String(
+		nsConfig.namespace+suffixIndexDateSeparator,
+		defaultIndexDateSeparator,
+		"Optional date separator of Jaeger indices. For example \".\" creates \"jaeger-span-2020.11.20 \". Default: '"+defaultIndexDateSeparator+"'")


you do not need to explain the default value in the description, it will be printed automatically

Sorry @sniperking1234 , this was my suggestion. Thanks, @yurishkuro, I should've picked up on that!

@albertteoh It doesn't matter. I've commit the code.

yurishkuro · 2020-12-02T03:57:21Z

I restarted the jobs.

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

yurishkuro

@albertteoh any more feedback?

albertteoh · 2020-12-03T04:52:39Z

@albertteoh any more feedback?

Nope, LGTM (besides needing a rebase), thanks @yurishkuro @sniperking1234.

Add elasticsearch index date format configuration

222dfef

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

sniperking1234 requested a review from a team as a code owner November 20, 2020 08:16

sniperking1234 requested a review from joe-elliott November 20, 2020 08:17

Merge branch 'master' into es_date_format

7aef0b3

albertteoh reviewed Nov 20, 2020

View reviewed changes

Set default separator to '-', remove 'none' config

07d7103

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

sniperking1234 requested a review from albertteoh November 23, 2020 07:42

albertteoh approved these changes Nov 23, 2020

View reviewed changes

Update options_test.go and separator usage.

4f722a1

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

sniperking1234 force-pushed the es_date_format branch from 986b44c to 4f722a1 Compare November 23, 2020 13:20

albertteoh reviewed Nov 23, 2020

View reviewed changes

Update options_test.go

7b4e2de

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

sniperking1234 force-pushed the es_date_format branch from c84eb95 to 7b4e2de Compare November 23, 2020 14:01

albertteoh approved these changes Nov 23, 2020

View reviewed changes

sniperking1234 mentioned this pull request Nov 25, 2020

Add elasticsearch storage date format config. jaegertracing/spark-dependencies#101

Merged

Merge branch 'master' into es_date_format

541fd59

sniperking1234 mentioned this pull request Dec 1, 2020

Add elasticsearch storage date format config. jaegertracing/jaeger-operator#1325

Merged

Merge branch 'master' into es_date_format

2718661

yurishkuro reviewed Dec 2, 2020

View reviewed changes

Remove separator's default value description.

d73a9e0

Signed-off-by: Chen Zhengwei <chenzhengwei@inspur.com> Signed-off-by: chen zhengwei <chenzhengwei@inspur.com>

albertteoh mentioned this pull request Dec 2, 2020

Fix issue for Elasticsearch CI failures #2678

Merged

Merge branch 'master' into es_date_format

21a34c0

yurishkuro approved these changes Dec 2, 2020

View reviewed changes

yurishkuro changed the title ~~Add elasticsearch storage date format config.~~ Support configurable date separator for Elasticsearch index names Dec 3, 2020

yurishkuro merged commit 6c2be45 into jaegertracing:master Dec 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support configurable date separator for Elasticsearch index names #2637

Support configurable date separator for Elasticsearch index names #2637

sniperking1234 commented Nov 20, 2020 •

edited by yurishkuro

Loading

codecov bot commented Nov 20, 2020 •

edited

Loading

albertteoh left a comment

albertteoh Nov 20, 2020

albertteoh Nov 20, 2020

albertteoh Nov 20, 2020

albertteoh Nov 20, 2020

sniperking1234 commented Nov 23, 2020

albertteoh left a comment •

edited

Loading

albertteoh Nov 23, 2020

albertteoh Nov 23, 2020 •

edited

Loading

albertteoh Nov 23, 2020

sniperking1234 Nov 23, 2020 •

edited

Loading

albertteoh left a comment

sniperking1234 commented Nov 24, 2020

yurishkuro commented Nov 24, 2020

sniperking1234 commented Dec 2, 2020

yurishkuro Dec 2, 2020

albertteoh Dec 2, 2020

sniperking1234 Dec 2, 2020

yurishkuro commented Dec 2, 2020

yurishkuro left a comment

albertteoh commented Dec 3, 2020 •

edited

Loading

Support configurable date separator for Elasticsearch index names #2637

Support configurable date separator for Elasticsearch index names #2637

Conversation

sniperking1234 commented Nov 20, 2020 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Nov 20, 2020 • edited Loading

Codecov Report

albertteoh left a 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

sniperking1234 commented Nov 23, 2020

albertteoh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sniperking1234 Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

albertteoh left a comment

Choose a reason for hiding this comment

sniperking1234 commented Nov 24, 2020

yurishkuro commented Nov 24, 2020

sniperking1234 commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Dec 2, 2020

yurishkuro left a comment

Choose a reason for hiding this comment

albertteoh commented Dec 3, 2020 • edited Loading

sniperking1234 commented Nov 20, 2020 •

edited by yurishkuro

Loading

codecov bot commented Nov 20, 2020 •

edited

Loading

albertteoh left a comment •

edited

Loading

albertteoh Nov 23, 2020 •

edited

Loading

sniperking1234 Nov 23, 2020 •

edited

Loading

albertteoh commented Dec 3, 2020 •

edited

Loading