Update chart to add metrics sql support #4277

AdityaHegde · 2024-03-08T11:43:53Z

This PR replaces the old queryName and queryArgs with metrics sql support.

New format,

data:
  metrics_sql: |
    SELECT domain, count(*) as impression
    FROM AdBids_model
    GROUP BY domain

data:
  api: adbids_per_publisher

New API,

➜  rill git:(main) curl http://localhost:9009/v1/instances/default/charts/adbids_domain_chart/data
[{"domain":"sports.yahoo.com","impression":12889},{"domain":"news.google.com","impression":12883},{"domain":"facebook.com","impression":15609},{"domain":"instagram.com","impression":13104},{"domain":"news.yahoo.com","impression":14870},{"domain":"msn.com","impression":15526},{"domain":"google.com","impression":15119}]

Note: if any chart was created, the db file will have to be deleted.

k-anshul · 2024-03-08T11:52:36Z

runtime/server/queries_charts.go

+	chartName := req.PathValue("chart_name")
+	// TODO: is a separate auth needed?
+	if !auth.GetClaims(ctx).CanInstance(instanceId, auth.ReadMetrics) {
+		return ErrForbidden


This returns grpc error which I think will map to a bad request. Should use httputil.Error.

k-anshul · 2024-03-08T11:53:32Z

runtime/server/queries_charts.go

+
+	ch, err := resolveChart(ctx, s.runtime, instanceId, chartName)
+	if err != nil {
+		return err


Same as above, it will again map to http.StatusBadRequest.

k-anshul · 2024-03-08T11:57:17Z

runtime/server/queries_charts.go

@@ -0,0 +1,116 @@
+package server


may be this should be charts_api.go >

k-anshul · 2024-03-08T11:58:01Z

runtime/server/server.go

@@ -208,6 +208,9 @@ func (s *Server) HTTPHandler(ctx context.Context, registerAdditionalHandlers fun
 	// custom API handler
 	observability.MuxHandle(httpMux, "/v1/instances/{instance_id}/api/{name...}", observability.Middleware("runtime", s.logger, auth.HTTPMiddleware(s.aud, httputil.Handler(s.apiForName))))

+	// custom Chart Data handler
+	observability.MuxHandle(httpMux, "/v1/instances/{instance_id}/charts/{chart_name...}", observability.Middleware("runtime", s.logger, auth.HTTPMiddleware(s.aud, httputil.Handler(s.GetChartData))))


Path also had /data suffix?

Ah yes will add that.

k-anshul · 2024-03-08T11:59:50Z

runtime/server/queries_charts.go

+func resolveChart(ctx context.Context, rt *runtime.Runtime, instanceID, name string) (*runtimev1.ChartSpec, error) {
+	ctrl, err := rt.Controller(ctx, instanceID)
+	if err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())


should not return grpc errors otherwise everything gets mapped to bad request (which although is correct in this case). See httputil.WriteError.

Ya updating this method to just return the error as is. The caller will handle the not found and to use httputil.Errorf

k-anshul · 2024-03-08T12:00:53Z

runtime/server/queries_charts.go

+
+	res, err := ctrl.Get(ctx, &runtimev1.ResourceName{Kind: runtime.ResourceKindChart, Name: name}, false)
+	if err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())


This should handle notfound case separately since any other error apart from that is an internal error.

begelundmuller · 2024-03-08T12:09:25Z

proto/rill/runtime/v1/resources.proto

+  string metrics_sql = 2;
+  string api = 3;
+  map<string, string> args = 4;


Instead of putting the YAML properties here, I would suggest having data_resolver and data_resolver_properties mirroring the APISpec. And then we should have one common YAML structure for parsing a data: clause (which we will want to use in all of APIs, charts, alerts, reports, etc.).

This would be similar to the parseScheduleYAML function (see here) in the compiler. We might have this:

type DataYAML struct { SQL string `...` MetricsSQL string `...` // Can add others. Like "api" for referencing another existing API. } func parseDataYAML(raw *DataYAML) (resolver string, resolverProps map[string]any, error) { ... } // In charts: type ChartYaml struct { // ... Data *DataYAML `yaml:"data"` } // In alerts: type AlertYAML struct { // ... Data *DataYAML `yaml:"data"` // This will eventually replace/deprecate the "query:" property } // In APIs: type APIYAML struct { // ... Data *DataYAML `yaml:",inline"` // Inline since API's don't nest under "data": } // etc.

begelundmuller · 2024-03-08T12:59:53Z

runtime/compilers/rillv1/parse_chart.go

-	if (tmp.Data.MetricsSQL == "" && tmp.Data.API == "") || (tmp.Data.MetricsSQL != "" && tmp.Data.API != "") {
-		return fmt.Errorf("exactly one of metrics_sql or api should be set")
+	if tmp.Data == nil {
+		return errors.New("data is mandatory")


Suggested change

return errors.New("data is mandatory")

return errors.New(`missing required property "data"`)

begelundmuller · 2024-03-08T13:01:09Z

runtime/compilers/rillv1/parse_chart.go

+	// Convert resolver properties to structpb.Struct before inserting the resource (since we can't return errors after that point)
+	resolverPropsPB, err := structpb.NewStruct(resolverProps)
+	if err != nil {
+		return fmt.Errorf("encountered invalid property type: %w", err)
+	}


Actually, can we also move this into the parseDataYAML function, and return a structpb value instead of a map? Since otherwise, we will need to duplicate it every place that has a data section.

begelundmuller · 2024-03-08T13:08:25Z

runtime/server/charts_api.go

+	switch resolver {
+	case "Metrics":
+		resolverPropsPB, err := structpb.NewStruct(map[string]interface{}{
+			"sql": resolverProps["sql"],
+		})
+		if err != nil {
+			return nil, err
+		}
+		// TODO: are other fields needed?
+		api = &runtimev1.API{
+			Spec: &runtimev1.APISpec{
+				Resolver:           "Metrics",
+				ResolverProperties: resolverPropsPB,
+			},
+		}
+
+	case "API":
+		apiName, ok := resolverProps["api"].(string)
+		if !ok {
+			return nil, errors.New("api name is missing")
+		}
+		api, err = rt.APIForName(ctx, instanceID, apiName)
+		if err != nil {
+			return nil, err
+		}
+
+	default:
+		return nil, errors.New("unknown resolver")
+	}


I would suggest refactoring runtime.APIResolverOptions to take Resolver and ResolverProperties as direct options instead of an API.

Since the point is to make the resolvers generic enough that all the code between the parser and the actual resolver doesn't need to know about the different available options.

To make that work, you probably also need to add a new resolver called "API" that proxies to another resolver (and maybe checks to avoid infinite recursion if you want to be fancy?)

Update chart to add metrics sql support

672be82

AdityaHegde requested a review from k-anshul March 8, 2024 11:45

k-anshul reviewed Mar 8, 2024

View reviewed changes

PR comments

0b715c2

AdityaHegde force-pushed the adityahegde/chart-metrics-sql branch from e8cdf75 to 0b715c2 Compare March 8, 2024 12:07

begelundmuller requested changes Mar 8, 2024

View reviewed changes

Use a common Data YML format

8ea285b

AdityaHegde force-pushed the adityahegde/chart-metrics-sql branch from 47841d8 to 8ea285b Compare March 8, 2024 12:41

begelundmuller requested changes Mar 8, 2024

View reviewed changes

begelundmuller added 2 commits March 8, 2024 17:53

Code review

fc3ee3b

Self review

908c498

begelundmuller approved these changes Mar 8, 2024

View reviewed changes

begelundmuller merged commit 00a4a18 into main Mar 8, 2024
7 checks passed

begelundmuller deleted the adityahegde/chart-metrics-sql branch March 8, 2024 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update chart to add metrics sql support #4277

Update chart to add metrics sql support #4277

AdityaHegde commented Mar 8, 2024 •

edited

Loading

k-anshul Mar 8, 2024

k-anshul Mar 8, 2024

k-anshul Mar 8, 2024

k-anshul Mar 8, 2024

AdityaHegde Mar 8, 2024

k-anshul Mar 8, 2024

AdityaHegde Mar 8, 2024

k-anshul Mar 8, 2024

begelundmuller Mar 8, 2024

begelundmuller Mar 8, 2024

begelundmuller Mar 8, 2024

begelundmuller Mar 8, 2024

begelundmuller Mar 8, 2024

	return errors.New("data is mandatory")
	return errors.New(`missing required property "data"`)

Update chart to add metrics sql support #4277

Update chart to add metrics sql support #4277

Conversation

AdityaHegde commented Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaHegde commented Mar 8, 2024 •

edited

Loading