partial support expand #663

guidao · 2022-01-11T12:16:12Z

Implement issue: #245

Civil · 2022-01-12T15:14:01Z

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)

Civil · 2022-01-12T15:14:41Z

Alternatively it might be worth do a find request and post-process the response, if of course find message have enough data.

guidao · 2022-01-13T03:20:30Z

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 Find function to implement a new Expand function, so I am using the MultiGlobRequest structure. So is reusing the MultiGlobRequest structure 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 return aaa.*.bbb.2 and aaa.*.bbb.3, it needs to return aaa.1.bbb.2 and aaa.2.bbb.3, maybe a new flag could be added to indicate whether it is Find or Expand?

Civil · 2022-01-13T11:24:18Z

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.

guidao · 2022-01-15T03:41:43Z

DeepSource execution failed, but it was not introduced by this pr. Should I fix it?

Civil · 2022-01-18T08:43:07Z

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)

guidao · 2022-01-18T12:16:23Z

Got it. I will only fix the problem introduced by this pr in order to avoid too many changes.

guidao · 2022-01-19T02:35:41Z

@Civil This pr is ready, can you help to review it?

Civil · 2022-01-19T09:04:35Z

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.

deniszh · 2022-06-10T15:24:18Z

@guidao: Are you OK if I pick up your PR? If you have any updates - it would be great. Thanks!

zipper/protocols/v3/protobuf_group.go

@@ -186,6 +186,36 @@
 	return &globs, stats, nil
 }

+func (c *ClientProtoV3Group) Expand(ctx context.Context, request *protov3.MultiGlobRequest) (*protov3.MultiGlobResponse, *types.Stats, merry.Error) {
+	logger := c.logger.With(zap.String("type", "expand"), zap.Strings("request", request.Metrics))


zipper/protocols/graphite/graphite_group.go

@@ -255,6 +258,79 @@
 	return &r, stats, nil
 }

+func (c *GraphiteGroup) Expand(ctx context.Context, request *protov3.MultiGlobRequest) (*protov3.MultiGlobResponse, *types.Stats, merry.Error) {
+	logger := c.logger.With(zap.String("type", "expand"), zap.Strings("request", request.Metrics))


zipper/protocols/prometheus/prometheus_group.go

@@ -494,6 +494,115 @@
 	return &r, stats, nil
 }

+func (c *PrometheusGroup) Expand(ctx context.Context, request *protov3.MultiGlobRequest) (*protov3.MultiGlobResponse, *types.Stats, merry.Error) {
+	logger := c.logger.With(zap.String("type", "expand"), zap.Strings("request", request.Metrics))


guidao · 2022-06-13T05:49:05Z

@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.

deniszh · 2022-12-16T13:09:06Z

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.

Civil · 2022-12-16T13:48:43Z

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.

deniszh · 2022-12-16T16:33:54Z

It has some usecases, like grafana/grafana#33694 - but not widely used, no.

deniszh · 2022-12-22T13:50:40Z

Closed in the favor of #748

guidao · 2022-12-23T03:16:49Z

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?

deniszh · 2022-12-23T10:56:54Z

@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?

deniszh · 2022-12-23T13:47:26Z

PS: you probably mean grafana/grafana#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.

guidao · 2022-12-24T00:39:42Z

PS: you probably mean grafana/grafana#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.

Good, I also think the new PR is better.

guidao force-pushed the feature/expand branch 2 times, most recently from 89bbb4f to 8fc9584 Compare January 14, 2022 07:10

guidao changed the title ~~WIP: partial support expand~~ partial support expand Jan 17, 2022

guidao force-pushed the feature/expand branch from 10e95d9 to 6c47d1d Compare January 18, 2022 08:32

partial support expand

0107a08

guidao force-pushed the feature/expand branch from 6c47d1d to 0107a08 Compare January 18, 2022 12:34

Merge branch 'main' into feature/expand

6fd4e6c

guidao closed this Mar 2, 2022

deniszh reopened this Jun 10, 2022

Merge branch 'main' into feature/expand

5758f99

github-advanced-security bot found potential problems Jun 10, 2022

View reviewed changes

deniszh and others added 2 commits November 4, 2022 14:22

Merge branch 'main' into feature/expand

b16fb12

Fixing type conversion

7863a6c

Civil force-pushed the main branch from 142a7f5 to 5a92ce1 Compare December 9, 2022 07:44

deniszh mentioned this pull request Dec 16, 2022

/metric/expand API implementation bookingcom/carbonapi#449

Merged

deniszh mentioned this pull request Dec 22, 2022

Implementing /metrics/expand API #748

Merged

deniszh closed this Dec 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

partial support expand #663

partial support expand #663

guidao commented Jan 11, 2022

Civil commented Jan 12, 2022

Civil commented Jan 12, 2022

guidao commented Jan 13, 2022 •

edited

Loading

Civil commented Jan 13, 2022

guidao commented Jan 15, 2022

Civil commented Jan 18, 2022 •

edited

Loading

guidao commented Jan 18, 2022

guidao commented Jan 19, 2022

Civil commented Jan 19, 2022

deniszh commented Jun 10, 2022 •

edited

Loading

guidao commented Jun 13, 2022

deniszh commented Dec 16, 2022

Civil commented Dec 16, 2022 •

edited

Loading

deniszh commented Dec 16, 2022

deniszh commented Dec 22, 2022

guidao commented Dec 23, 2022 •

edited

Loading

deniszh commented Dec 23, 2022

deniszh commented Dec 23, 2022

guidao commented Dec 24, 2022

partial support expand #663

partial support expand #663

Conversation

guidao commented Jan 11, 2022

Civil commented Jan 12, 2022

Civil commented Jan 12, 2022

guidao commented Jan 13, 2022 • edited Loading

Civil commented Jan 13, 2022

guidao commented Jan 15, 2022

Civil commented Jan 18, 2022 • edited Loading

guidao commented Jan 18, 2022

guidao commented Jan 19, 2022

Civil commented Jan 19, 2022

deniszh commented Jun 10, 2022 • edited Loading

guidao commented Jun 13, 2022

deniszh commented Dec 16, 2022

Civil commented Dec 16, 2022 • edited Loading

deniszh commented Dec 16, 2022

deniszh commented Dec 22, 2022

guidao commented Dec 23, 2022 • edited Loading

deniszh commented Dec 23, 2022

deniszh commented Dec 23, 2022

guidao commented Dec 24, 2022

guidao commented Jan 13, 2022 •

edited

Loading

Civil commented Jan 18, 2022 •

edited

Loading

deniszh commented Jun 10, 2022 •

edited

Loading

Civil commented Dec 16, 2022 •

edited

Loading

guidao commented Dec 23, 2022 •

edited

Loading