Change index prefix from : to -

[pavolloffay]

Resolves #1238

Spark dependencies: jaegertracing/spark-dependencies#52

The readers will still read from indices containing prefix "prefix:"
to keep backward compability.

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

[codecov]

Codecov Report

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

@@          Coverage Diff           @@
##           master   #1284   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         161     161           
  Lines        7206    7210    +4     
======================================
+ Hits         7206    7210    +4

Impacted Files	Coverage Δ
plugin/storage/es/spanstore/writer.go	100% <100%> (ø)	⬆️
plugin/storage/es/options.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 3a3a5e0...f0f26b8. Read the comment docs.

[pavolloffay]

cc @jaegertracing/elasticsearch

[yurishkuro]

I assume there are will be some additional migration steps people will need to take, e.g. to configure rotations / cleaner to look for different names?

Also, a meta comment: I would much prefer if we didn't have to deal with these separators in the first place. The --es-index-prefix parameter should be truly the prefix, not the prefix's prefix, i.e. the user should specify "production-" as the value, and then our code would not have to deal with this : vs. - difference at all.

Is it possible to make such change (remove separators from code) and have a reasonable migration procedure, e.g. by creating aliases with the new separator for the old aliases and then simply re-deploying collectors with the updated prefix value?

[objectiser]

@pavolloffay I think Yuri has a point about the index prefix should be the full prefix, without adding an extra separator. Maybe we could add a temporary command line flag, e.g. es-old-index-prefix where the user could specify the old index name (with or without the :) separator? This way, the additional indexes are only included as long as the user needs the old indices included.

However this would be more disruptive - so the current PR provides the smoothest migration path.

[pavolloffay]

@yurishkuro I had exactly the same comment in #1238 (comment). I am keen on this approach adding the separator in other projects is annoying. However this will mean a potential breaking change. If we go with plain prefix without separator can we release as 1.9? We want to release on Monday so please comment.

What I have done at the moment does not require any migration. The only thing to keep an eye on is to use the older version of es-index-cleaner to delete the old indices.

If we go with the plain prefix - without adding a separator it will require a migration (If users want to migrate to -, otherwise then can add : to prefix). Users will have to create aliases e.g. prod-jaeger-span-2019-01-17 pointing to prod:jaeger-span-2019-01-17 for all indices they would like to read from - depending on --es.max-span-age (default 3 days/indices).

Maybe we could add a temporary command line flag, e.g. es-old-index-prefix where the user could specify the old index name (with or without the :) separator?

• It can be done with the plain prefix without separator users will add : at the end of the prefix. The flag would not be enabled by default so it requires some configuration one way or another.

[yurishkuro]

Another option is to keep this hardcoded separator for now, do a release without serious migration requirements, and then in the next release remove the separator. Next release will be breaking for configuration, but with easy migration since by then old index names would hopefully TTL-out

[pavolloffay]

@yurishkuro I am not sure if I am following you. Could you please be specific about what you are proposing?

Another option is to keep this hardcoded separator for now,

So noop in this release? Or keep the current state of this PR?

Next release will be breaking for configuration, but with easy migration since by then old index names would hopefully TTL-out

So the next release remove the separator and require it to be added by users?
TTL-out depends on how users are configuring max-span-age and if they migrate version by version which might not be always true.

[yurishkuro]

I am suggesting that we merge your current PR version which changes the hardcoded separator to - and maintains backwards compatibility. As I understand from your comments, this is pretty much backwards compatible, so we can release it as 1.9 without breaking changes.

Then we can make the change for 1.10 where the separator is not hardcoded, and people would need to change their deployment configuration to pass production- instead of production as a prefix. This would be a breaking change, but if people already deployed 1.9 and all their historical indices by then have been already renamed to use - prefix, then the migration from 1.9 -> 1.10 is still very straightforward.

We can also punt on removing the default separator altogether, to reduce churn, maybe reserve it for some future 2.0 release.