[query] Refactor query code, add warnings to prom output #2265

arnikola · 2020-04-14T21:24:04Z

No description provided.

robskillington · 2020-04-14T21:56:21Z

src/query/api/v1/handler/close.go

+
+func (c *ctxCanceller) WatchForCancel(
+	ctx context.Context,
+	cancel context.CancelFunc,


Probably don't need cancel func here since not used?

robskillington · 2020-04-16T15:20:19Z

src/query/api/v1/handler/prometheus/native/common_test.go

+		"warnings": [
+			"foo_bar",
+			"baz_qux"
+		],


robskillington · 2020-04-16T15:21:39Z

src/query/api/v1/handler/prometheus/native/read.go

+		promReadMetrics: newPromReadMetrics(taggedScope),
+		// timeoutOps:          opts.TimeoutOpts(),
+		// keepEmpty:           opts.Config().ResultOptions.KeepNans,
+		// instrumentOpts:      opts.InstrumentOpts(),


Believe can remove the comments now?

robskillington · 2020-04-16T15:24:43Z

src/query/api/v1/handler/prometheus/native/read.go

 	return h
 }

 func (h *PromReadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	timer := h.promReadMetrics.fetchTimerSuccess.Start()
-	fetchOpts, rErr := h.fetchOptionsBuilder.NewFetchOptions(r)
+	defer timer.Stop()


robskillington · 2020-04-16T15:25:25Z

src/query/api/v1/handler/prometheus/native/read.go

+	ctx := context.WithValue(r.Context(), handler.HeaderKey, r.Header)
+	logger := logging.WithContext(ctx, h.opts.InstrumentOpts())
+
+	ParsedOptions, rErr := ParseRequest(ctx, r, h.opts)


This is a local var, so can just be lower camel cased yeah? i.e. parsedOpts

Oops, must have gotten dragged into a rename

robskillington · 2020-04-16T15:28:35Z

src/query/api/v1/handler/prometheus/native/read.go

+		opentracingext.Error.Set(sp, true)
+		logger.Error("range query error",
+			zap.Error(err),
+			zap.Any("ParsedOptions", ParsedOptions))


Name here I assume can be zap.Any("parsedOpts", parsedOpts)

robskillington · 2020-04-16T15:31:44Z

src/query/api/v1/handler/prometheus/native/read.go

-	numSteps := int64(params.End.Sub(params.Start) / params.Step)
-	maxComputedDatapoints := h.limitsCfg.MaxComputedDatapoints()
-	if maxComputedDatapoints > 0 && numSteps > maxComputedDatapoints {
+func validateRequest(params *models.RequestParams, maxPoints int) error {


Does params necessarily need to be a pointer type? Could it be just params models.RequestParams (I notice we do &params when we call this which might be unnecessary)?

Don't think so; just kept the types as is generally

robskillington · 2020-04-16T15:32:42Z

src/query/api/v1/handler/prometheus/native/read_common.go

@@ -77,6 +72,12 @@ func read(
 		return emptyResult, err
 	}

+	// Detect clients closing connections.
+	if cancelWatcher != nil {
+		ctx, cancel := context.WithTimeout(ctx, fetchOpts.Timeout)


Do we want to defer cancel() here?

robskillington · 2020-04-16T15:44:24Z

src/query/api/v1/handler/prometheus/native/read.go

-	w.Header().Set("Content-Type", "application/json")
-	handleroptions.AddWarningHeaders(w, result.meta)
-	return result.series, params, nil
+	return ParsedOptions{


Any reason not to create the cancel watcher here instead of later and tacking onto the ParsedOptions

I'm trying to avoid taking a http.ResponseWriter into the read function

robskillington · 2020-04-16T16:01:32Z

src/query/api/v1/handler/prometheus/native/read_instantaneous_test.go

@@ -159,7 +170,8 @@ func testPromReadInstantHandler(
 			]
 		}
 	}
-	`, at0.Unix(), value0, at1.Unix(), value1))
+	`, jsonWarnings, at0.Unix(), value0, at1.Unix(), value1))


Maybe we should get rid of the fmt.Sprintf stuff and introduce types JSONMap, JSONArray and then add a MustPrettyJSONObject(t, ....).

I prototyped this here, looks easy to drop in I think?
https://play.golang.org/p/sTatAL6vaEE

Hah, nice :)

Actually, wouldn't using maps lead to mismatches in data? Also, just from my experience doing it in the last few minutes, building these structures seems more error prone than just writing up the expected JSON?

If we MustPrettyJSON both of them, then they should get the correct ordering as per JSON pretty printing behavior, should actually mean less possibility of writing a wrong JSON output matcher.

robskillington · 2020-04-16T16:03:05Z

src/query/api/v1/handler/prometheus/remote/read.go

 	}

 	data, err := proto.Marshal(resp)
 	if err != nil {
-		h.promReadMetrics.fetchErrorsServer.Inc(1)
+		// h.promReadMetrics.fetchErrorsServer.Inc(1)


Can restore this?

The metrics will get incremented in the calling function, which has the actual metrics

robskillington · 2020-04-16T16:04:00Z

src/query/api/v1/handler/prometheus/remote/read.go

-			result, err := h.engine.ExecuteProm(ctx, query, queryOpts, fetchOpts)
+			// Detect clients closing connections.
+			if cancelWatcher != nil {
+				cancelWatcher.WatchForCancel(ctx, cancel)


Seems like we pepper cancel watchers in a ton of places, is there a way to basically make this middleware instead?

Just seems very messy the setup/tear down of it (even though its useful itself).

Yeah; I only saw these in a few places tbh; wouldn't be opposed to pulling it into some sort of middleware

Might add a todo for this?

…rnikola/handler-refactor

robskillington

LGTM

arnikola added 6 commits April 9, 2020 13:26

[query] Remote read handler refactor

6e2e205

revert read a

ae57c4b

stuff

741cff4

ss

c14f18b

Refactor

91e0530

More refactor

3993d8f

robskillington changed the title ~~[query] refactor query code, add warnings to prom output~~ [query] Refactor query code, add warnings to prom output Apr 14, 2020

robskillington reviewed Apr 14, 2020

View reviewed changes

arnikola added 2 commits April 14, 2020 18:10

Test updates

d28e141

Merge branch 'master' into arnikola/handler-refactor

02d0e24

robskillington mentioned this pull request Apr 15, 2020

WIP [query] Add debug params to Prometheus Remote Read to return raw query data for PromQL queries as input as well as JSON formatted results #2267

Closed

6 tasks

arnikola added 2 commits April 15, 2020 13:52

Fix handler generation

cca9425

Refactored native read a bit

786ec4d

robskillington reviewed Apr 16, 2020

View reviewed changes

src/query/api/v1/handler/prometheus/native/common_test.go Outdated

"warnings": [

"foo_bar",

"baz_qux"

],

Copy link

Collaborator

robskillington Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

robskillington reviewed Apr 16, 2020

View reviewed changes

arnikola added 2 commits April 16, 2020 13:25

PR response, further refactor

258eb93

Fix racy test

b6688ec

arnikola and others added 3 commits April 16, 2020 17:21

Add todo

446fdcc

Merge branch 'master' into arnikola/handler-refactor

412227c

Use JSONMap and JSONArray instead of string subtitutions

c7e3946

robskillington and others added 3 commits April 17, 2020 10:32

Merge branch 'master' into arnikola/handler-refactor

1b465b6

Move import

d60744e

Merge branch 'arnikola/handler-refactor' of github.com:m3db/m3 into a…

7c9481c

…rnikola/handler-refactor

robskillington approved these changes Apr 17, 2020

View reviewed changes

arnikola merged commit eea6199 into master Apr 17, 2020

justinjc deleted the arnikola/handler-refactor branch May 14, 2020 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Refactor query code, add warnings to prom output #2265

[query] Refactor query code, add warnings to prom output #2265

arnikola commented Apr 14, 2020

robskillington Apr 14, 2020

robskillington Apr 16, 2020

robskillington Apr 16, 2020

robskillington Apr 16, 2020

robskillington Apr 16, 2020

arnikola Apr 16, 2020

robskillington Apr 16, 2020

robskillington Apr 16, 2020

arnikola Apr 16, 2020

robskillington Apr 16, 2020

robskillington Apr 16, 2020

arnikola Apr 16, 2020

robskillington Apr 16, 2020

arnikola Apr 16, 2020

arnikola Apr 16, 2020

robskillington Apr 16, 2020 •

edited

Loading

robskillington Apr 16, 2020

arnikola Apr 16, 2020 •

edited

Loading

robskillington Apr 16, 2020

arnikola Apr 16, 2020

arnikola Apr 16, 2020

robskillington left a comment

[query] Refactor query code, add warnings to prom output #2265

[query] Refactor query code, add warnings to prom output #2265

Conversation

arnikola commented Apr 14, 2020

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

robskillington Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola Apr 16, 2020 • 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

robskillington left a comment

Choose a reason for hiding this comment

robskillington Apr 16, 2020 •

edited

Loading

arnikola Apr 16, 2020 •

edited

Loading