Adds Cassandra support for Autocomplete tags

[zeagord]

No description provided.

[codefromthecrypt]

good start... next in-memory I think as then we can add some tests

[codefromthecrypt]

possible refactor possibility, as I recognise this class :P

[michaelsembwever]

I don't know what the cardinality of tags will be? But this won't scale.

But, for example, if there's 1 million tags in a Zipkin storage this full-table scan is going to be painful (if not timeout).

[jcchavezs]

Right. I would do a progressive query (by pagination but only simple one, no ordering and deifnirively not with offset but with after.

[drolando]

If we want to have a whitelist of tags to index and not just index all tags you might get away without doing this at all. And just return the list of whitelisted tags.

[zeagord]

Added the whitelist implementation. Let me know your thoughts.

[zeagord]

I have added the in-memory api. I will revisit the cassandra tomorrow and address the above concerns.

[codefromthecrypt]

made a comment about fixed cardinality.. we definitely need to document this as it is indeed inappropriate for unbounded. #2236 (comment)

One thing @zeagord and I discussed is initially inheriting the config for the other names (service/span). This is for simplicity. In the future we could add a timestamp/lookback parameter to only fetch the values for a range. However, same problem would apply to service/span so thinking of that later

[zeagord]

Need to add tests and revisit the Elastic search design.

[tacigar]

In elasticsearch, how about using _q instead of changing mapping?

[codefromthecrypt]

reuse _q field in elasticsearch

we can think about it but the performance might be bad. for example, getting the key names would require an expression I am not sure how to express unless we hard code the possible key names. if we hard code the possible key names yes it could work, but we have to check the performance of scanning all span documents to get the values

[codefromthecrypt]

good news is we can try it. actually I think we need to hard code key names anyway especially in Cassandra.

…

On Mon, 10 Dec 2018, 08:27 Adrian Cole ***@***.*** wrote: > reuse _q field in elasticsearch we can think about it but the performance might be bad. for example, getting the key names would require an expression I am not sure how to express unless we hard code the possible key names. if we hard code the possible key names yes it could work, but we have to check the performance of scanning all span documents to get the values

[codefromthecrypt]

so on cassandra (and elasticsearch) we'll need to ensure the "deduper" is in use to avoid thrashing writes.

In both cases, it might be helpful to reverse-engineer the service-span mapping to re-use the same table. ex PRIMARY KEY ((type, key), value) This could make data management in general easier long term.

Since there is a time bomb on elasticsearch #2219, we might want to solve that first before merging this (or at least before cutting a release with it).

Meanwhile, we can allow UI testing to work with static managed list of tags. (ex there may be only several values associated with phase, for example.. so one way is to allow the UI to configure predefined where it is small)

[zeagord]

is this tagkey because "key" is reserved? I suspect if not easier to just do key/value

Bodyconverters for ES looks for "key" from the result of aggregations which clashes with the key in the tag {k,v}. It could be the name of the aggregation. I will change the name of the aggs alone and see if it works.

[drolando]

If we want to have a whitelist of tags to index and not just index all tags you might get away without doing this at all. And just return the list of whitelisted tags.

[michaelsembwever]

In both cases, it might be helpful to reverse-engineer the service-span mapping to re-use the same table. ex PRIMARY KEY ((type, key), value) This could make data management in general easier long term.

if service-span and tags are collapsed to one table it does make full table scans (eg QueryBuilder.select("key").distinct().from(TABLE__)) more painful…
i suspect keeping two separate tables is in fact wiser, thanks to @drolando for raising this.

[codefromthecrypt]

made most of a pass!

[codefromthecrypt]

this chops off the basic functionality and will allow the UI work to start immediately when merged. I can help rework the other commits similarly #2332

[codefromthecrypt]

other storage impls pulled out in #2333 and #2334

[codefromthecrypt]

I think this is nearly ready. we need to test the auto-upgrade logic and also update the README files to talk about how autocomplete works

[michaelsembwever]

Code changes LGTM.

One comment though: most of the diff is whitespace/code-style changes. For the reviewer there's a huge waste of time reading diffs that have nothing to do with the actual PR. It would be great if those changes where separated out to a separate follow-up commit (still within the PR). That way by reviewing just the first commit of the PR a lot of time could be saved.

[codefromthecrypt]

@michaelsembwever sorry about the formatting thing. I was in a rush to get something stable before I turned off internet for the vacation, but that amplified efforts to others.. not sure the better call but I apologize nevertheless. thanks for reviewing despite this.

[codefromthecrypt]

FYI travis is failing still on the same tests as last push this needs to be looked into prior to merge