Make ES index name configurable #1009

pavolloffay · 2018-08-20T18:51:26Z

Resolves #799

Dependency job PR: jaegertracing/spark-dependencies#38

pavolloffay · 2018-08-20T18:59:17Z

Prefix foo- creates foo-jaeger-<type>-<date>. The prefix is added directly to the name, we could add separator - or :.

We could make jaeger the default prefix. But I think it's better to keep fixed jaeger in the index name.

codecov · 2018-08-21T08:14:35Z

Codecov Report

Merging #1009 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1009   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         139     139           
  Lines        6399    6414   +15     
======================================
+ Hits         6399    6414   +15

Impacted Files	Coverage Δ
plugin/storage/es/options.go	`100% <100%> (ø)`	⬆️
plugin/storage/es/dependencystore/storage.go	`100% <100%> (ø)`	⬆️
plugin/storage/es/spanstore/writer.go	`100% <100%> (ø)`	⬆️
plugin/storage/es/factory.go	`100% <100%> (ø)`	⬆️
plugin/storage/es/spanstore/reader.go	`100% <100%> (ø)`	⬆️

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 40a3cc9...ddd8ad0. Read the comment docs.

jpkrohling

Looks sane to me, but I'm not up to speed with the storage parts.

pavolloffay · 2018-08-21T10:12:57Z

I have changed a couple of things.

It 's backward compatible - if there is no index prefix it uses the same naming pattern jaeger-<type>..
if the index prefix is specified it adds : at the end. e.g. prefix foo creates foo:jaeger-<type>..

pavolloffay · 2018-08-21T14:35:38Z

@yurishkuro @black-adder @vprithvi would you like to have a look?

yurishkuro · 2018-08-21T14:54:52Z

plugin/storage/es/dependencystore/storage.go

-func indexName(date time.Time) string {
-	return dependencyIndexPrefix + date.UTC().Format("2006-01-02")
+func indexName(prefix string, date time.Time) string {
+	return prefix + dependencyIndex + date.UTC().Format("2006-01-02")


nit: we're doing this for every span write? lots of GC pressure. At least we can pre-concatenate the prefix with the constant string in the constructor. Ideally we would cache the mapping from date (sans time) to string.

It's per span write. Will update not sure if the cache would make a significant difference

yurishkuro · 2018-08-21T14:55:44Z

plugin/storage/es/esCleaner.py

@@ -8,16 +8,23 @@

 def main():
    if len(sys.argv) == 1:
-        print('USAGE: [TIMEOUT=(default 120)] %s NUM_OF_DAYS HOSTNAME[:PORT] ...' % sys.argv[0])
+        print('USAGE: [TIMEOUT=(default 120)] [INDEX_PREFIX=(default \'\')] %s NUM_OF_DAYS HOSTNAME[:PORT] ...' % sys.argv[0])


could just say default ""

yurishkuro · 2018-08-21T14:58:45Z

plugin/storage/es/factory.go

@@ -74,16 +74,16 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger)
 // CreateSpanReader implements storage.Factory
 func (f *Factory) CreateSpanReader() (spanstore.Reader, error) {
 	cfg := f.primaryConfig
-	return esSpanStore.NewSpanReader(f.primaryClient, f.logger, cfg.GetMaxSpanAge(), f.metricsFactory), nil
+	return esSpanStore.NewSpanReader(f.primaryClient, f.logger, cfg.GetMaxSpanAge(), f.metricsFactory, cfg.GetIndexPrefix()), nil


I think this reached the point of needing its own type SpanReaderParams struct{}

I will keep the current form for the simplicity. If it grows we can refactor

yurishkuro · 2018-08-21T15:00:28Z

plugin/storage/es/options.go

+	flagSet.String(
+		nsConfig.namespace+suffixIndexPrefix,
+		nsConfig.IndexPrefix,
+		"The prefix of Jaeger indices. For example \"production\" creates \"production:jaeger-*\". (Default unset)")


nit: Optional prefix for index names. (don't need "Default unset")

yurishkuro · 2018-08-21T15:04:35Z

plugin/storage/es/spanstore/reader.go

@@ -147,33 +152,33 @@ func (s *SpanReader) unmarshalJSONSpan(esSpanRaw *elastic.SearchHit) (*jModel.Sp
 }

 // Returns the array of indices that we need to query, based on query params
-func findIndices(prefix string, startTime time.Time, endTime time.Time) []string {
+func (s *SpanReader) findIndices(indexName string, startTime time.Time, endTime time.Time) []string {


nit: change method name, e.g. indicesForTimeRange

It allow simple multitenancy - index per tenant or k8s namespace. Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Make ES index name configurable It allow simple multitenancy - index per tenant or k8s namespace. Signed-off-by: Pavol Loffay <ploffay@redhat.com> * docs Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix comment Signed-off-by: Pavol Loffay <ploffay@redhat.com> * index prefix Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix index cleaner and add prefix delimiter Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix dependency storage Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fmt and help message Signed-off-by: Pavol Loffay <ploffay@redhat.com> * increase converage Signed-off-by: Pavol Loffay <ploffay@redhat.com> * review fixes Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay requested review from black-adder, jpkrohling, vprithvi and yurishkuro as code owners August 20, 2018 18:51

ghost assigned pavolloffay Aug 20, 2018

ghost added the review label Aug 20, 2018

pavolloffay force-pushed the es-index-configurable branch from 3aed9d5 to 69b7216 Compare August 20, 2018 18:53

jpkrohling approved these changes Aug 21, 2018

View reviewed changes

pavolloffay mentioned this pull request Aug 21, 2018

Make ES index prefix configurable jaegertracing/spark-dependencies#38

Merged

yurishkuro reviewed Aug 21, 2018

View reviewed changes

pavolloffay added 9 commits August 22, 2018 11:23

Make ES index name configurable

b0d6620

It allow simple multitenancy - index per tenant or k8s namespace. Signed-off-by: Pavol Loffay <ploffay@redhat.com>

docs

db0aba4

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix comment

06f5f69

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

index prefix

69ccc0b

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix index cleaner and add prefix delimiter

df96e31

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix dependency storage

9fcaa4c

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fmt and help message

9e81555

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

increase converage

d913b29

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

review fixes

ddd8ad0

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the es-index-configurable branch from f08f10e to ddd8ad0 Compare August 22, 2018 09:23

pavolloffay merged commit 75af597 into jaegertracing:master Aug 22, 2018

ghost removed the review label Aug 22, 2018

pavolloffay added the storage/elasticsearch label Sep 4, 2018

pavolloffay mentioned this pull request Sep 4, 2018

Allow storing tags as object fields in ES - better kibana support #1018

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ES index name configurable #1009

Make ES index name configurable #1009

pavolloffay commented Aug 20, 2018 •

edited

Loading

pavolloffay commented Aug 20, 2018 •

edited

Loading

codecov bot commented Aug 21, 2018 •

edited

Loading

jpkrohling left a comment

pavolloffay commented Aug 21, 2018

pavolloffay commented Aug 21, 2018

yurishkuro Aug 21, 2018

pavolloffay Aug 21, 2018

yurishkuro Aug 21, 2018

yurishkuro Aug 21, 2018

pavolloffay Aug 22, 2018

yurishkuro Aug 21, 2018

yurishkuro Aug 21, 2018

Make ES index name configurable #1009

Make ES index name configurable #1009

Conversation

pavolloffay commented Aug 20, 2018 • edited Loading

pavolloffay commented Aug 20, 2018 • edited Loading

codecov bot commented Aug 21, 2018 • edited Loading

Codecov Report

jpkrohling left a comment

Choose a reason for hiding this comment

pavolloffay commented Aug 21, 2018

pavolloffay commented Aug 21, 2018

yurishkuro Aug 21, 2018

Choose a reason for hiding this comment

pavolloffay Aug 21, 2018

Choose a reason for hiding this comment

yurishkuro Aug 21, 2018

Choose a reason for hiding this comment

yurishkuro Aug 21, 2018

Choose a reason for hiding this comment

pavolloffay Aug 22, 2018

Choose a reason for hiding this comment

yurishkuro Aug 21, 2018

Choose a reason for hiding this comment

yurishkuro Aug 21, 2018

Choose a reason for hiding this comment

pavolloffay commented Aug 20, 2018 •

edited

Loading

pavolloffay commented Aug 20, 2018 •

edited

Loading

codecov bot commented Aug 21, 2018 •

edited

Loading