Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Istio tags cannot be queried for #550

Closed
yurishkuro opened this issue Nov 20, 2017 · 18 comments
Closed

Istio tags cannot be queried for #550

yurishkuro opened this issue Nov 20, 2017 · 18 comments
Labels

Comments

@yurishkuro
Copy link
Member

Istio creates tags in the form "guid:x-request-id" = "some guid". Because the name contains a colon, there is no way to search for these tags from the UI, because the URL syntax expected by the query service is tag=key:value

Any reason why we shouldn't be using = as key/value separator?

@black-adder
Copy link
Contributor

or provide a way to escape it? since we can potentially get "guid=x-request-id":"some guid"

@objectiser
Copy link
Contributor

+1 to = as key/value separator - it is more natural.

@yurishkuro
Copy link
Member Author

There's really no good character to use as a separator because sometimes people want to search by the URL, and both : and = can be easily found in the URLs.

@tiffon any thoughts on best practices for REST API for this scenario? Maybe we should be simply sending a small JSON snipper in the URL when these special characters are found in the tag key or value? And if they are not, then fall back onto the simple tag=urlencode(key=value)

@tiffon
Copy link
Member

tiffon commented Nov 20, 2017

@yurishkuro Using JSON is definitely an option and just boils down to deserializing twice. This approach is very flexible.

As a reference, the equivalent JSON and resultant query string value:

{"guid:x-request-id": "some guid"}
tags=%7B%22tag-guid%3Ax-request-id%22%3A%20%22some%20guid%22%7D

A user-friendly approach for entering tags into the search form might be to use logfmt (go lib), which would circumvent the ambiguity issues

Form input:

guid:x-request-id="some guid" url="https//ok.com?a=b"

JSON:

{"guid:x-request-id": "some guid", "url": "https//ok.com?a=b"}

URL encoded JSON:

%7B%22guid%3Ax-request-id%22%3A%20%22some%20guid%22%2C%20%22url%22%3A%20%22https%2F%2Fok.com%3Fa%3Db%22%7D

@yurishkuro
Copy link
Member Author

@tiffon logfmt for input would be awesome, much friendlier than what we have today (with colons and pipes). Would you also need a respective parsing lib for the UI?

Does it make sense to even bother with more readable URL format when there are no special characters in the tag key/value? E.g. right now you can send a URL like ?tag=http.status_code:200

@tiffon
Copy link
Member

tiffon commented Nov 20, 2017

@ys Yes, a logfmt lib for the UI would be necessary.

There is logfmt. It might need some work to get it running in the browser.

I think that it makes sense to always URI encode JSON or logfmt for tags regardless of whether they have special characters or not.

We could either stick with logfmt from start to finish or only use logfmt for the form input format and send URI encoded JSON over the wire.

@yurishkuro
Copy link
Member Author

I don't have strong feeling about doing the parsing in the UI or the server. Currently the server API is well-structured, but it already performs parsing of key:value encoding for tags, and would have to do even more parsing if we send JSON as tag query. Since there's a Go library for it, might as well parse the logfmt on the server (not that different from Prometheus parsing query expressions).

What do others think? @pavolloffay @black-adder @objectiser @isaachier @vprithvi

@objectiser
Copy link
Contributor

No strong opinion but sounds like parsing logfmt on the server sounds better.

@yurishkuro yurishkuro added the ui label Nov 26, 2017
@tiffon
Copy link
Member

tiffon commented Nov 28, 2017

The interest in logfmt is based primarily on UX concerns, and it seems odd for this to drive the serialization format the server is expecting.

I'm fine either way, but it seems practical to pass data to the server in a format that is wider use such as URI encoded JSON or base64 encoded JSON.

@rbtcollins
Copy link
Contributor

I would like to see the proposed improvements. re: performance, the double-parsing isn't a concern IMO: scaling out the query component is trivial, and reads generally have a human driving them, so its O(staff), not O(span-rate)

@tiffon
Copy link
Member

tiffon commented Dec 19, 2017

The only reason logfmt is in the picture, as opposed to some other format, is because it's an easier format to enter values. It's driven by a UX consideration. IMO, the query-service should accept JSON, that way if we change the UI we can still just send JSON to the backend.

@yurishkuro
Copy link
Member Author

I can add an extra query param tags that will expect a JSON string

@yurishkuro
Copy link
Member Author

yurishkuro commented Dec 19, 2017

@tiffon can we make sure that the tags JSON is always a string:string mapping? It's much easier to parse in Go as map[string]string, but if some values are non-strings (numbers, bools), then parsing becomes a lot harder messier.

@black-adder
Copy link
Contributor

I dont recall but won't this mean we will no longer support searching tag values that are not strings? Did we never support this?

@yurishkuro
Copy link
Member Author

This can be extended in the future, but today we don't support that anyway, the query API only allows for "exact match".

@yurishkuro
Copy link
Member Author

@tiffon over to you, the query service now accepts tags={"k": "v", ...}, as long as all values are "quoted strings".

@tiffon
Copy link
Member

tiffon commented Dec 19, 2017

@yurishkuro Got it. string:string shouldn't be a problem.

Created jaegertracing/jaeger-ui#145 to track this.

@yurishkuro
Copy link
Member Author

Will be released in 1.1.0 (#612)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants