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

[HWKMETRICS-523] Add the option to perform tags queries via JSON Path… #706

Closed
wants to merge 9 commits into from

Conversation

stefannegrea
Copy link
Contributor

… for metric tags. To use this feature, users need to prefix their tag query with "json:". JSON and regular tag queries can be mixed in a single request, the reply will just merge the results.

… for metric tags. To use this feature, users need to prefix their tag query with "json:". JSON and regular tag queries can be mixed in a single request, the reply will just merge the results.
@burmanm
Copy link
Contributor

burmanm commented Dec 2, 2016

I don't understand the purpose of this PR or this feature at all. What's the point of storing JSON as tag values?

@burmanm
Copy link
Contributor

burmanm commented Dec 2, 2016

And to previous comment, there was no issue in the ticket that says what feature is missing. If we want to store multiple values, we should do that - adding JSON parsing to it is a hack. Adding new querying capabilities should be planned somehow. And not that next time we add SQL to tags, which supports something else. Not to mention, JSONPath is not a standard or even popular.

@stefannegrea
Copy link
Contributor Author

@burmanm, there was a lot of discussion about what to do to better support complex tag values and querying the information stored. There were even design review sessions for this feature and all the feedback that I got pointed to JSON being the ideal candidate to store complex tag values. But that was possible even before this PR. This PR just adds an alternative to regex querying and nothing more.

JsonPath is not a standard (yet?) but it is widely used and it has good client libraries, tooling, and syntax. Do you know of anything better in this space?

@burmanm
Copy link
Contributor

burmanm commented Dec 2, 2016

I don't think I've seen such design review. It's a point solution, not suitable for our querying capability requirements. The design document should be attached to the JIRA-ticket in any case, if there is such.

What we'll at least some point need is possibility to use some sort of query language, that allows merging series, allows selecting the aggregations the user needs and so on. This should happen in the same way, not that we have somewhere JSON, next aggregation selection is XPath (which at least is somewhat standard) and series merging is perl.

That's a mess. And JSONPath is less standard than even JSON Pointers (RFC 6901), but does anyone even use that? It's a silly hack of XPath without the capabilities of XML. And that's the problem with all JSON notations, none of them are standard or widely used. Each library uses a different method.

@stefannegrea
Copy link
Contributor Author

@burmanm, using the current framework, we could support Json Path and Json Pointers simultaneously without any problems. The current regex implementation received a lot of negative feedback. We need something to complement that feature. This proposal does not take away regex nor does it prevent additional query languages.

@jsanda
Copy link
Contributor

jsanda commented Dec 2, 2016

I think the purpose of this PR is pretty well covered in the ticket's description. Using regex for label queries is overly complex and error prone.

I agree that it would be good to make any design doc(s) public and attached to or referenced in the ticket.

With respect to the popularity of JSONPath, I don't have a sense of that one way or another. It would be good to know though. If anyone has anything to share that would be helpful.

JSONPath might not be a standard, but there are existing libraries, tools, and documentation. If we go the route of building our own query language, I doubt we will be implementing some standard, and it will be completely unfamiliar to users.

I also think it would be good to identify what other use cases, features, etc. we want to support before making a final decision on how to proceed. I am opposed to introducing one solution to support one use case and then other solutions for other use cases.

At a minimum this PR is good in that it is starting some more active discussion.

@jsanda
Copy link
Contributor

jsanda commented Dec 8, 2016

I think it is interesting and worth noting that kubernetes has [some support0(http://kubernetes.io/docs/user-guide/jsonpath/) for jsonpath, and from some aos-devel mailing list questions, it likes you can use jsonpath with the oc command. If that is the case, then I think we have a stronger case for using jsonpath.

…ration tests for the REST API. All these tests cover a good portion of the matching capabilities for JSON Path.
@stefannegrea
Copy link
Contributor Author

I added a lot more tests (REST API and core service) to showcase JSON Path's query/matching syntax.

For usage examples please review the new test code committed with this PR.

Stefan Negrea added 7 commits December 15, 2016 16:37
…JsonPath tag query expressions are sent then take the intersection of both sets and return only metrics that match all tag queries.

Also, allow a tag key to be sent multiple times in the tag query. This allows splitting complex large tag queries for a single key/value pair to be broken into smaller expressions.
…s. That will make the syntax equivalent with JsonPath variant. Keep it optional for now to maintain backwards compatibility.
@stefannegrea
Copy link
Contributor Author

A newer implementation for this PR has been proposed. Closing this PR for now.

@stefannegrea stefannegrea deleted the HWKMETRICS-523 branch April 3, 2017 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants