feat: generic api to run promql query for metrics #2073

adarsh0728 · 2024-09-19T14:21:55Z

The PR aims to provide a generic API to run PromQL to fetch metrics for the UI.

The overall goal is to improve debuggability experience for platform and application engineers.

Signed-off-by: adarsh0728 <[email protected]>

yhl25 · 2024-09-25T09:49:09Z

server/apis/v1/handler.go

+
+func buildTimeSeriesData(ctx context.Context, v1api v1.API, data MetricSpecData, query string, format PrometheusResponseFormat) []any {
+	var timeSeriesData = make([]any, 0)


add code comments

Signed-off-by: adarsh0728 <[email protected]>

veds-g · 2024-09-25T13:01:53Z

server/apis/v1/handler.go

+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	var timeSeriesData = make([][]float64, 0)


please fix lint errors

veds-g · 2024-09-25T13:06:27Z

server/routes/routes.go

+	// Get the time series data for a specific metric.
+	r.POST("/namespaces/:namespace/pipelines/:pipeline/getMetricData", handler.GetMetricData)


are we planning to cover monovertex with the same endpoint?

as of now, planning to have a separate endpoint for monovertex (for same endpoint we may have to add another field in the body and that could make API more cluttered). Any suggestions are welcome

lets update the comment to include for pipeline for now.

Can we provide a query generic query service which works for any metrics?

Signed-off-by: adarsh0728 <[email protected]>

codecov · 2024-09-25T13:38:51Z

Codecov Report

Attention: Patch coverage is 0.68027% with 146 lines in your changes missing coverage. Please review.

Project coverage is 63.39%. Comparing base (3dbed43) to head (6e91261).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
server/apis/v1/promql_builder.go	0.00%	64 Missing ⚠️
server/apis/v1/handler.go	0.00%	47 Missing ⚠️
server/apis/v1/response_metrics.go	0.00%	34 Missing ⚠️
server/routes/routes.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2073      +/-   ##
==========================================
- Coverage   64.25%   63.39%   -0.87%     
==========================================
  Files         324      327       +3     
  Lines       30650    31354     +704     
==========================================
+ Hits        19695    19877     +182     
- Misses       9913    10435     +522     
  Partials     1042     1042

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yhl25 · 2024-09-25T13:54:13Z

server/apis/v1/response_metrics.go

+
+type MetricMetaData struct {
+	NumaMetricName string


Suggested change

type MetricMetaData struct {

NumaMetricName string

type MetricMetadata struct {

NumaMetricName string

yhl25 · 2024-09-25T14:06:24Z

server/apis/v1/handler.go

+	client, err := api.NewClient(api.Config{
+		Address: h.prometheusServerUrl,
+	})


We will end up creating a new client for every api invocation. Can we store it inside the handler struct and reuse it?

yes, will resolve this.

yhl25 · 2024-09-25T14:07:07Z

server/apis/v1/handler.go

+	v1api := v1.NewAPI(client)
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()


Same here. Lets not create a new one for every invocation.

Also make sure they are safe to be used across go routines.

veds-g · 2024-09-25T16:46:48Z

Lets add a flag in config map for server.disable.prometheusChart, similar to server.readonly. Pass it via /sysinfo, so that it can be used in UI to decide when to render charts.

whynowy · 2024-09-25T17:45:47Z

cmd/commands/server.go

@@ -80,5 +82,7 @@ func NewServerCommand() *cobra.Command {
 	command.Flags().StringVar(&serverAddr, "server-addr", sharedutil.LookupEnvStringOr("NUMAFLOW_SERVER_ADDRESS", "https://localhost:8443"), "The external address of the Numaflow server.")
 	command.Flags().StringVar(&corsAllowedOrigins, "cors-allowed-origins", sharedutil.LookupEnvStringOr("NUMAFLOW_SERVER_CORS_ALLOWED_ORIGINS", ""), "The values for allowed cors AllowOrigins header field, separated by comma.")
 	command.Flags().StringVar(&daemonClientProtocol, "daemon-client-protocol", sharedutil.LookupEnvStringOr("NUMAFLOW_SERVER_DAEMON_CLIENT_PROTOCOL", "grpc"), "The protocol used to connect to the Pipeline daemon service from Numaflow UX server, defaults to 'grpc'.")
+	command.Flags().StringVar(&prometheusServerUrl, "prometheus-server-url", sharedutil.LookupEnvStringOr("NUMAFLOW_SERVER_PROMETHEUS_URL", ""), "The prometheus server url for querying metrics data.")


Instead of using args, please mount the configmap as a volume.

whynowy · 2024-09-25T17:45:57Z

config/advanced-install/namespaced-numaflow-server.yaml

@@ -129,6 +129,12 @@ metadata:
  name: numaflow-cmd-params-config
 ---
 apiVersion: v1
+data: null


whynowy · 2024-09-25T17:50:24Z

server/routes/routes.go

+	// Get the time series data for a specific metric.
+	r.POST("/namespaces/:namespace/pipelines/:pipeline/getMetricData", handler.GetMetricData)


Can we provide a query generic query service which works for any metrics?

Signed-off-by: adarsh0728 <[email protected]>

whynowy · 2024-10-01T17:21:33Z

server/apis/v1/response_metrics.go

+
+type PrometheusClient struct {
+	// prometheus metric config from yaml
+	ConfigData []map[string]any


Abstract to structs.

whynowy · 2024-10-01T17:21:42Z

server/apis/v1/response_metrics.go

+	// prometheus server url in the config
+	ServerUrl string `yaml:"url"`
+	// patterns in the config
+	Patterns []map[string]any `yaml:"patterns"`


Abstract to structs.

whynowy · 2024-10-01T17:23:42Z

server/routes/routes.go

@@ -165,6 +165,8 @@ func v1Routes(ctx context.Context, r gin.IRouter, dexObj *v1.DexObject, localUse
 	r.GET("/namespaces/:namespace/mono-vertices/:mono-vertex/metrics", handler.GetMonoVertexMetrics)
 	// Get the health information of a mono vertex.
 	r.GET("/namespaces/:namespace/mono-vertices/:mono-vertex/health", handler.GetMonoVertexHealth)
+	// Get the time series data for a specific metric.
+	r.POST("/metricData", handler.GetMetricData)


/metrics-proxy

whynowy · 2024-10-01T17:25:18Z

server/cmd/server/start.go

@@ -206,5 +206,6 @@ func CreateAuthRouteMap(baseHref string) authz.RouteMap {
 		"GET:" + baseHref + "api/v1/namespaces/:namespace/mono-vertices/:mono-vertex/metrics":        authz.NewRouteInfo(authz.ObjectMonoVertex, true),
 		"POST:" + baseHref + "api/v1/namespaces/:namespace/mono-vertices":                            authz.NewRouteInfo(authz.ObjectMonoVertex, true),
 		"GET:" + baseHref + "api/v1/namespaces/:namespace/mono-vertices/:mono-vertex/health":         authz.NewRouteInfo(authz.ObjectMonoVertex, true),
+		"POST:" + baseHref + "api/v1/metricData":                                                     authz.NewRouteInfo(authz.ObjectAll, true),


@kohlisid - please review

config/namespace-install.yaml

whynowy · 2024-10-01T17:27:21Z

config/base/numaflow-server/numaflow-metric-server-config.yaml

+  name: numaflow-metric-server-config
+data:
+  config.yaml: |
+    url: http://my-release-kube-prometheus-prometheus.default.svc.cluster.local:9090


Give commented out examples.

… clarity Signed-off-by: adarsh0728 <[email protected]>

Signed-off-by: adarsh0728 <[email protected]>

whynowy · 2024-10-03T03:07:02Z

server/apis/v1/handler.go

+	var fieldsMap = make(map[string]any)
+
+	var (
+		requestBody     map[string]any


Shall me abstract the input data into a struct? map[string]any means no restriction.

whynowy · 2024-10-03T03:43:36Z

server/apis/v1/response_metrics.go

+	"gopkg.in/yaml.v2"
+)
+
+type PrometheusClient struct {


This struct has no method, maybe there's a better way abstract it?

kohlisid · 2024-10-03T05:07:05Z

server/apis/v1/response_metrics.go

+	)
+
+	// read prometheus metric config yaml from volume mount path
+	data, err = os.ReadFile("/etc/numaflow/metrics/config.yaml")


We can make this file path into a constant/config instead of hardcoding.

kohlisid · 2024-10-03T05:22:17Z

server/apis/v1/handler.go

+	}
+
+	// get expression for the pattern
+	expr := pattern.Expression


Maybe don't need this, we are checkingpattern.Expressiondirectly above as well, can be passed to subMatch directly.

kohlisid · 2024-10-03T05:23:47Z

server/apis/v1/handler.go

+
+	// find index and pattern from config based on req pattern name
+	for i, p := range h.prometheusClient.ConfigData {
+		if p.Name == requestBody["name"] {


Can we have a case where requestBody["name"] does not exist?

kohlisid · 2024-10-03T05:28:36Z

server/apis/v1/handler.go

+	}
+
+	// throw error if pattern name not found in the config
+	if index == -1 {


We can directly compare with pattern != nil right for the case it is not found.
Index might not be required here, I don't see it getting further in the code. Is that correct?

kohlisid · 2024-10-03T05:47:43Z

server/apis/v1/handler.go

+			return fmt.Sprintf("%f", v)
+		case int64:
+			return fmt.Sprintf("%d", v)
+		case map[string]any:


Is any allowed? Do we have any specific types that we support?

Signed-off-by: adarsh0728 <[email protected]>

vigith · 2024-10-03T17:16:10Z

config/advanced-install/namespaced-numaflow-server.yaml

+      rangeVector: 5m
+      aggregator: sum


is this overridable?

Signed-off-by: adarsh0728 <[email protected]>

whynowy · 2024-10-04T15:09:22Z

I created a feature branch dbt, instead of attempting merging to main, please merge it to the feature branch. After getting the e2e working (frontend, backend), then create a PR to main.

Signed-off-by: adarsh0728 <[email protected]>

adarsh0728 · 2024-10-14T10:40:27Z

Merging this into feature branch. Will create separate PRs for other issues. Once e2e is done, will create a PR into main.

wip: generic api to run promql query

28b7ca0

Signed-off-by: adarsh0728 <[email protected]>

adarsh0728 linked an issue Sep 19, 2024 that may be closed by this pull request

Backend: define an API to run PromQL query from the Numaflow Server #2058

Closed

POST request instead of GET in the routes map

403edbf

Signed-off-by: adarsh0728 <[email protected]>

adarsh0728 changed the title ~~feat #2058: generic api to run promql query for metrics~~ feat: generic api to run promql query for metrics Sep 19, 2024

adarsh0728 added 3 commits September 23, 2024 14:31

Merge branch 'main' into query-metrics

6db86ae

config changes: pick promehteus server url from config

0d5ac1b

Signed-off-by: adarsh0728 <[email protected]>

Merge branch 'main' into query-metrics

3d110dc

yhl25 reviewed Sep 25, 2024

View reviewed changes

adarsh0728 added 2 commits September 25, 2024 18:20

error handling and added counter metrics to map

60b9bc3

Signed-off-by: adarsh0728 <[email protected]>

handled lint warning

0af4076

Signed-off-by: adarsh0728 <[email protected]>

veds-g reviewed Sep 25, 2024

View reviewed changes

fix: unit test

a2af479

Signed-off-by: adarsh0728 <[email protected]>

adarsh0728 marked this pull request as ready for review September 25, 2024 13:46

adarsh0728 requested review from whynowy and vigith as code owners September 25, 2024 13:46

yhl25 reviewed Sep 25, 2024

View reviewed changes

whynowy reviewed Sep 25, 2024

View reviewed changes

adarsh0728 added 4 commits September 27, 2024 13:58

Merge branch 'main' into query-metrics

2933002

new design impl

97f0631

Signed-off-by: adarsh0728 <[email protected]>

Merge branch 'main' into query-metrics

2b38568

minor fixes

348cce1

Signed-off-by: adarsh0728 <[email protected]>

adarsh0728 requested a review from yhl25 October 1, 2024 11:45

whynowy reviewed Oct 1, 2024

View reviewed changes

whynowy marked this pull request as draft October 2, 2024 06:07

adarsh0728 added 2 commits October 2, 2024 22:39

Refactor: Map abstracted to struct and updated route/config names for…

c641f84

… clarity Signed-off-by: adarsh0728 <[email protected]>

merge main branch

8b494f8

Signed-off-by: adarsh0728 <[email protected]>

whynowy reviewed Oct 3, 2024

View reviewed changes

kohlisid reviewed Oct 3, 2024

View reviewed changes

feat: new promql builder implementation

2c68985

Signed-off-by: adarsh0728 <[email protected]>

vigith reviewed Oct 3, 2024

View reviewed changes

revert last commit and refactoring

6e91261

Signed-off-by: adarsh0728 <[email protected]>

adarsh0728 changed the base branch from main to dbt October 7, 2024 03:13

adarsh0728 added 4 commits October 7, 2024 08:44

Merge branch 'dbt' into query-metrics

b3e76ba

interfaces: promql as a service

7bb09f2

Signed-off-by: adarsh0728 <[email protected]>

refactor: promql builder to promql service

1063201

Signed-off-by: adarsh0728 <[email protected]>

added unit tests for prom ql service

02aa2b6

Signed-off-by: adarsh0728 <[email protected]>

vigith marked this pull request as ready for review October 9, 2024 15:41

adarsh0728 merged commit e56b41b into numaproj:dbt Oct 14, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: generic api to run promql query for metrics #2073

feat: generic api to run promql query for metrics #2073

adarsh0728 commented Sep 19, 2024 •

edited

Loading

yhl25 Sep 25, 2024

veds-g Sep 25, 2024 •

edited

Loading

adarsh0728 Sep 25, 2024

veds-g Sep 25, 2024

adarsh0728 Sep 25, 2024

veds-g Sep 25, 2024

whynowy Sep 25, 2024

codecov bot commented Sep 25, 2024 •

edited

Loading

yhl25 Sep 25, 2024

yhl25 Sep 25, 2024

adarsh0728 Sep 25, 2024

yhl25 Sep 25, 2024

yhl25 Sep 25, 2024 •

edited

Loading

veds-g commented Sep 25, 2024

whynowy Sep 25, 2024

whynowy Sep 25, 2024

whynowy Sep 25, 2024

whynowy Oct 1, 2024

whynowy Oct 1, 2024

whynowy Oct 1, 2024

whynowy Oct 1, 2024

whynowy Oct 1, 2024

whynowy Oct 3, 2024

whynowy Oct 3, 2024

kohlisid Oct 3, 2024

kohlisid Oct 3, 2024

kohlisid Oct 3, 2024

kohlisid Oct 3, 2024

kohlisid Oct 3, 2024

vigith Oct 3, 2024

whynowy commented Oct 4, 2024

adarsh0728 commented Oct 14, 2024


		func buildTimeSeriesData(ctx context.Context, v1api v1.API, data MetricSpecData, query string, format PrometheusResponseFormat) []any {
		var timeSeriesData = make([]any, 0)

		// Get the time series data for a specific metric.
		r.POST("/namespaces/:namespace/pipelines/:pipeline/getMetricData", handler.GetMetricData)

feat: generic api to run promql query for metrics #2073

feat: generic api to run promql query for metrics #2073

Conversation

adarsh0728 commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

veds-g Sep 25, 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

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

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

yhl25 Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

veds-g commented Sep 25, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whynowy commented Oct 4, 2024

adarsh0728 commented Oct 14, 2024

adarsh0728 commented Sep 19, 2024 •

edited

Loading

veds-g Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

yhl25 Sep 25, 2024 •

edited

Loading