carbonapi
carbonapi copied to clipboard
go-graphite
partial support expand
I guess as the title have "WIP" in the name it is considered to be work-in-progress (that means I won't review it in-depth).
One note, however. Current PR expect backend to have native expand handler. However I currently do not aware of any backend that implements that.
If you have your own custom backend, please consider implementing some sort of feature flags that would mark that backend implements that type of query. For example carbonapi_v3_pb have a support for Capability Message, so you should send a PR to protocols repo here adding there ExpandMsg support (also feel free to add specific data types to make it easier to use, of course if it's actually needed) and then in your PR bump a revision of protocols repo required and guard that handler with capability request (like how it's done for Victoria Metrics backend)
Alternatively it might be worth do a find request and post-process the response, if of course find message have enough data.
Thank you for your comments. I would like to implement this feature, but have some confusion and would like to ask for your help.
- The current pr implementation is copying most of the code of the
Findfunction to implement a newExpandfunction, so I am using theMultiGlobRequeststructure. So is reusing theMultiGlobRequeststructure an acceptable or comparable way to do this? Or is it better to define a new similar structure? - By default the find request does not get enough information, for example for the metrics (aaa.1.bbb.2 and aaa.2.bbb.3)
aaa.*.bbb.*will returnaaa.*.bbb.2andaaa.*.bbb.3, it needs to returnaaa.1.bbb.2 and aaa.2.bbb.3, maybe a new flag could be added to indicate whether it is Find or Expand?
structure an acceptable or comparable way to do this?
I guess in that case it is ok to use the same structure as for find.
By default the find request does not get enough information
In that case the most effective approach would be to implement that on a backend side and indeed use a different handler on a backend.
Generic approach here is also possible but it would be less effective, e.x. doing requests for each and every glob that you see and implement all that logic on carbonapi side.
Overall it might be easier to approach it from a backend side (I'm not sure what you are using - go-carbon or graphite-clickhouse or something else as a backend).
Once you have a working backend it will be easier to test your changes on carbonapi side.
DeepSource execution failed, but it was not introduced by this pr. Should I fix it?
About DeepSource - sometimes it provide false-positive (pointing to problems that were already there or ignoring exclusion of vendored dependencies). In that case you can leave it as-is.
However sometimes it detects something new - in that case it would be great if you'll be able to fix it before you mark the PR as ready (when it detects something new sometimes it shows all the problems that were there even in vendored deps, so unfortunately it's not very easy to read)
Got it. I will only fix the problem introduced by this pr in order to avoid too many changes.
I will likely have some time reading it during the weekend and I'll try not to forget that I have it waiting for the review.
@guidao: Are you OK if I pick up your PR? If you have any updates - it would be great. Thanks!
@guidao: Are you OK if I pick up your PR? If you have any updates - it would be great. Thanks!
Please feel free to do so. We solved this problem using other means, so there is nothing new in this PR.
FYI here: IMO this approach is very flexible and gives ability to override expand in every backend but IMO it's a bit overkill, I'll create separate PR similar to https://github.com/bookingcom/carbonapi/pull/449 - which will just implement separate encoder for expand results, but results will be collected by same call as find does. IMO that should be good enough implementation.
Thanks. Yeah, as it seems not a lot of people need it, it doesn't need to be especially efficient if that would take too much time to implement.
It has some usecases, like https://github.com/grafana/grafana/pull/33694 - but not widely used, no.
FYI here: IMO this approach is very flexible and gives ability to override expand in every backend but IMO it's a bit overkill, I'll create separate PR similar to bookingcom/carbonapi#449 - which will just implement separate encoder for expand results, but results will be collected by same call as find does. IMO that should be good enough implementation.
It's been a bit long and my memory is a bit fuzzy, I remember I also tried to implement Expend through the Find interface. But there was some semantics that could not be covered. Does it support expand(*.servers.*.cpu) in this way?
@guidao : well, in graphite-web it's implemented in exactly that way - just another wrapper around same results which find returns. Could you please elaborate why it should have different semantics and how exactly expand(*.servers.*.cpu) should be supported?
PS: you probably mean https://github.com/grafana/grafana/pull/33694 ? I implemented having exact that use case in mind: find?query=*.servers.*.cpu will return cpu, expand?query=*.servers.*.cpu will return full names.
PS: you probably mean grafana/grafana#33694 ? I implemented having exact that use case in mind:
find?query=*.servers.*.cpuwill returncpu,expand?query=*.servers.*.cpuwill return full names.
Good, I also think the new PR is better.