Add HTTP handler for metrics querying #3095

albertteoh · 2021-06-16T13:07:04Z

Signed-off-by: albertteoh albert.teoh@logz.io

Which problem is this PR solving?

Relates to "Monitor" tab for service health metrics #2954.

Short description of the changes

Adds the HTTP handlers for metrics querying.

Signed-off-by: albertteoh <albert.teoh@logz.io>

cmd/query/app/handler_archive_test.go

cmd/query/app/http_handler.go

codecov · 2021-06-16T13:14:18Z

Codecov Report

Merging #3095 (fddc433) into master (13885e5) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3095      +/-   ##
==========================================
+ Coverage   95.86%   95.96%   +0.09%     
==========================================
  Files         235      236       +1     
  Lines       10143    10252     +109     
==========================================
+ Hits         9724     9838     +114     
+ Misses        349      344       -5     
  Partials       70       70

Impacted Files	Coverage Δ
cmd/query/app/handler_options.go	`100.00% <100.00%> (ø)`
cmd/query/app/http_handler.go	`100.00% <100.00%> (ø)`
cmd/query/app/json_marshaler.go	`100.00% <100.00%> (ø)`
cmd/query/app/query_parser.go	`100.00% <100.00%> (ø)`
cmd/query/app/server.go	`97.22% <100.00%> (+0.01%)`	⬆️
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️
...lugin/sampling/strategystore/adaptive/processor.go	`100.00% <0.00%> (+0.92%)`	⬆️
pkg/config/tlscfg/cert_watcher.go	`94.80% <0.00%> (+2.59%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13885e5...fddc433. Read the comment docs.

yurishkuro · 2021-06-16T17:17:39Z

cmd/query/app/http_handler.go

@@ -115,6 +127,10 @@ func (aH *APIHandler) RegisterRoutes(router *mux.Router) {
 	// TODO - remove this when UI catches up
 	aH.handleFunc(router, aH.getOperationsLegacy, "/services/{%s}/operations", serviceParam).Methods(http.MethodGet)
 	aH.handleFunc(router, aH.dependencies, "/dependencies").Methods(http.MethodGet)
+	aH.handleFunc(router, aH.latencies, "/metrics/latencies/{%s}", servicesParam).Methods(http.MethodGet)


Are you particularly set on passing service in the URL? We tried to move away from that (see L126)

Nope, I'm not particularly attached to passing service as a URL path segment. Just thought that having services in the URL path is an affordance to suggest it is a mandatory parameter.

What was the reason behind moving away from it? I only found this comment, but couldn't find the discussion on why there was final agreement on moving in this direction: #52 (comment)

What was the reason behind moving away from it?

part of my holy war on high cardinality REST URLs (when params are encoded in the URL we can't use URL as the span name)

when the service name is part of the URL people sometimes tend to think it can go there unencoded, which breaks when the service name contains a slash

in the end though - not very strong reasons

cmd/query/app/json_marshaler.go

cmd/query/app/http_handler.go

Signed-off-by: albertteoh <albert.teoh@logz.io>

cmd/query/app/http_handler.go

albertteoh · 2021-06-17T13:20:26Z

cmd/query/app/query_parser.go

 	service := r.FormValue(serviceParam)
 	operation := r.FormValue(operationParam)

-	startTime, err := p.parseTime(startTimeParam, r)
+	startTime, err := p.parseTime(r, startTimeParam, time.Microsecond)


the search endpoint defines times in microseconds since epoch (why?), but other endpoints such as dependencies and metrics use milliseconds since epoch, since those are the time units JS (i.e. Jaeger UI) use e.g. Date.now(), so we need to pass the units to the parseTime function to support both cases.

We should add this to comments in the code. The search operates on span latencies, which are expressed as microseconds in the data model, so it makes sense to support high accuracy. The search UI itself does not insist on exact units because it supports string like 1ms. We had a debate over whether units should be handled by the UI instead of the backend service, but here we are, since Go makes parsing 1ms very easy.

The dependencies API does not operate on the latency space, instead its timestamps are just time range selections, and the typical backend granularity of those is on the order of 15min or more. So microseconds aren't really that useful in this domain, although I certainly would've preferred having a consistent time representation.

Metrics is a new domain, you need to decide which representation makes more sense. I think it's closer to dependencies, where you're asking about wall clock time, not the latency domain.

The search operates on span latencies, which are expressed as microseconds in the data model

Out of curiosity, why micros and not posix nanos which I think also fit into an int64? Were there some limitations in storage or clients to handle posix nanos?

Metrics is a new domain, you need to decide which representation makes more sense. I think it's closer to dependencies, where you're asking about wall clock time, not the latency domain.

I think milliseconds are most appropriate (I've documented these reasons in comments within queryParser):

The main client in mind is Jaeger UI. Being a react.js app, its built-in time precision is milliseconds so it simplifies usage for Jaeger UI.

The min step size is 1ms for the Prometheus-compliant storage backends I've tested.

It also follows that data point timestamps are reported with a millisecond precision, at least from my tests so far.

Out of curiosity, why micros and not posix nanos which I think also fit into an int64? Were there some limitations in storage or clients to handle posix nanos?

legacy from zipkin origins, which still only supports micros, whereas our domain model uses Go's time.Time which has nanos. But our Thrift API uses micros.

cmd/query/app/query_parser.go

albertteoh · 2021-06-17T13:23:33Z

cmd/query/app/query_parser.go

+
+	// If no units are supplied, assume parsing of duration strings like 5ms.
+	case units == nil:
+		if d, err = time.ParseDuration(formValue); err == nil {


the search endpoint defines durations as duration strings, presumably to support human-friendly clients like curl, etc. so this maintains support for such duration parameter values.

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh

I've tidied a few things up:

use encapsulation for duration parsing
move query param parsing logic for each endpoint into queryParser
fix up comments
unexport unnecessarily exported var
wrap errors

PTAL.

cmd/query/app/query_parser.go

cmd/query/app/http_handler.go

cmd/query/app/query_parser.go

yurishkuro · 2021-06-19T15:05:08Z

cmd/query/app/query_parser.go

 	service := r.FormValue(serviceParam)
 	operation := r.FormValue(operationParam)

-	startTime, err := p.parseTime(startTimeParam, r)
+	startTime, err := p.parseTime(r, startTimeParam, time.Microsecond)


We should add this to comments in the code. The search operates on span latencies, which are expressed as microseconds in the data model, so it makes sense to support high accuracy. The search UI itself does not insist on exact units because it supports string like 1ms. We had a debate over whether units should be handled by the UI instead of the backend service, but here we are, since Go makes parsing 1ms very easy.

The dependencies API does not operate on the latency space, instead its timestamps are just time range selections, and the typical backend granularity of those is on the order of 15min or more. So microseconds aren't really that useful in this domain, although I certainly would've preferred having a consistent time representation.

Metrics is a new domain, you need to decide which representation makes more sense. I think it's closer to dependencies, where you're asking about wall clock time, not the latency domain.

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh · 2021-06-20T01:44:31Z

cmd/query/app/query_parser.go

-// parse takes a request and constructs a model of parameters
+// parseTraceQueryParams takes a request and constructs a model of parameters.
+//
+// Why start/end parameters are expressed in microseconds:


@yurishkuro I've added your comments here for posterity. Please advise if they would be better placed else where.

albertteoh · 2021-06-20T01:48:20Z

cmd/query/app/query_parser.go

+
+// parseMetricsQueryParams takes a request and constructs a model of metrics query parameters.
+//
+// Why the API is designed using an end time (endTs) and lookback:


Also added reasons for various parameter decisions here. Again, if you can think of a better place to put these, I can move them.

albertteoh · 2021-06-20T01:51:17Z

cmd/query/app/http_handler_test.go

-		spanstore.OperationQueryParameters{ServiceName: "abc/trifle"},
+		spanstore.OperationQueryParameters{
+			ServiceName: "abc/trifle",
+			SpanKind:    "server",


here's the assertion to catch my earlier breaking API change.

Signed-off-by: albertteoh <albert.teoh@logz.io>

yurishkuro · 2021-06-20T20:18:49Z

cmd/query/app/query_parser.go

@@ -76,15 +121,16 @@ type traceQueryParameters struct {
 //     key := strValue
 //     keyValue := strValue ':' strValue
 //     tags :== 'tags=' jsonMap
-func (p *queryParser) parse(r *http.Request) (*traceQueryParameters, error) {
+func (p *queryParser) parseTraceQueryParams(r *http.Request) (*traceQueryParameters, error) {
+	dp := durationStringParser{}


declared too far from usage

yurishkuro · 2021-06-20T20:19:41Z

cmd/query/app/query_parser.go

+		lookback time.Duration
+	}
+
+	durationParser interface {


embrace functional programming - this is just a function, and the two parsers can be easily implemented to return functions, without using any structs

yurishkuro · 2021-06-20T20:23:51Z

cmd/query/app/query_parser.go

+// Valid input span kinds are the string representations from the OpenTelemetry model/proto/metrics/otelspankind.proto.
+// For example:
+// - "SPAN_KIND_SERVER"
+// - "SPAN_KIND_CLIENT"


is this consistent with the strings we used in the Operations API? I thought we used strings client/server, as defined in OpenTracing.

Good pickup, I couldn't find formal docs in comments but found evidence where "server" and "client" are used in the operations API unit tests.

Added a mapping from jaeger/opentracing span kinds to OTEL span kinds.

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh · 2021-06-21T00:21:39Z

cmd/query/app/json_marshaler.go

+	"github.com/gogo/protobuf/proto"
+)
+
+type jsonMarshaler = func(writer io.Writer, response interface{}) error


I decided to follow your advice and embrace functional programming here as well; it looks much more concise :)

albertteoh · 2021-06-21T00:24:46Z

cmd/query/app/query_parser.go

+// Valid input span kinds are the string representations from the OpenTelemetry model/proto/metrics/otelspankind.proto.
+// For example:
+// - "SPAN_KIND_SERVER"
+// - "SPAN_KIND_CLIENT"


Good pickup, I couldn't find formal docs in comments but found evidence where "server" and "client" are used in the operations API unit tests.

Added a mapping from jaeger/opentracing span kinds to OTEL span kinds.

albertteoh · 2021-06-21T00:32:02Z

cmd/query/app/query_parser.go

+		lookback time.Duration
+	}
+
+	durationParser = func(s string) (time.Duration, error)


converted the duration parsers to functions. I did initially try to adopt functional programming but was thrown by the fact that the unit duration parser needed to maintain state (the duration units), but hadn't thought that we can get around this via closures.

Thanks for pointing this out, @yurishkuro, it looks a lot neater, and I've learnt something new! :)

cmd/query/app/query_parser_test.go

cmd/query/app/query_parser.go

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: albertteoh <albert.teoh@logz.io>

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh · 2021-06-21T04:37:45Z

Thanks for the review, Yuri!

Add HTTP handler

4abadc8

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh commented Jun 16, 2021

View reviewed changes

cmd/query/app/handler_archive_test.go Show resolved Hide resolved

cmd/query/app/http_handler.go Outdated Show resolved Hide resolved

cmd/query/app/http_handler.go Outdated Show resolved Hide resolved

albertteoh marked this pull request as ready for review June 16, 2021 13:18

albertteoh requested a review from a team as a code owner June 16, 2021 13:18

albertteoh requested a review from vprithvi June 16, 2021 13:18

yurishkuro reviewed Jun 16, 2021

View reviewed changes

cmd/query/app/json_marshaler.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jun 16, 2021

View reviewed changes

cmd/query/app/http_handler.go Outdated Show resolved Hide resolved

albertteoh mentioned this pull request Jun 17, 2021

"Monitor" tab for service health metrics #2954

Closed

14 tasks

albertteoh added 3 commits June 17, 2021 23:08

Address review comments

35d4171

Signed-off-by: albertteoh <albert.teoh@logz.io>

Merge branch 'master' into 2954-http-handler

adacf51

Better func name

df06e60

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh commented Jun 17, 2021

View reviewed changes

albertteoh added 3 commits June 19, 2021 20:43

Tidy up comments, functions and vars

9f222e5

Signed-off-by: albertteoh <albert.teoh@logz.io>

Wrap error string

f8834d7

Signed-off-by: albertteoh <albert.teoh@logz.io>

Merge branch 'master' into 2954-http-handler

9d326d7

albertteoh commented Jun 19, 2021

View reviewed changes

cmd/query/app/query_parser.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jun 19, 2021

View reviewed changes

albertteoh added 2 commits June 20, 2021 11:23

Address review comments

0c02b4b

Signed-off-by: albertteoh <albert.teoh@logz.io>

nit: Revert ordering of params

8fb87df

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh commented Jun 20, 2021

View reviewed changes

albertteoh added 2 commits June 20, 2021 18:21

Improve comments

e305859

Signed-off-by: albertteoh <albert.teoh@logz.io>

More correct metrics query BNF syntax

ba4234d

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh force-pushed the 2954-http-handler branch from b04a2f7 to ba4234d Compare June 20, 2021 08:25

yurishkuro reviewed Jun 20, 2021

View reviewed changes

Address review comments

afdaeb0

Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh commented Jun 21, 2021

View reviewed changes

yurishkuro reviewed Jun 21, 2021

View reviewed changes

cmd/query/app/query_parser_test.go Outdated Show resolved Hide resolved

cmd/query/app/query_parser.go Outdated Show resolved Hide resolved

cmd/query/app/query_parser.go Outdated Show resolved Hide resolved

Apply suggestions from code review

d9f5740

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: albertteoh <albert.teoh@logz.io>

albertteoh force-pushed the 2954-http-handler branch from 679fd00 to d9f5740 Compare June 21, 2021 02:29

Fix tests

fddc433

Signed-off-by: albertteoh <albert.teoh@logz.io>

yurishkuro approved these changes Jun 21, 2021

View reviewed changes

yurishkuro merged commit 681dd68 into jaegertracing:master Jun 21, 2021

albertteoh deleted the 2954-http-handler branch June 21, 2021 04:37

jpkrohling added this to the Release 1.24.0 milestone Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HTTP handler for metrics querying #3095

Add HTTP handler for metrics querying #3095

albertteoh commented Jun 16, 2021

codecov bot commented Jun 16, 2021 •

edited

Loading

yurishkuro Jun 16, 2021

albertteoh Jun 16, 2021

yurishkuro Jun 17, 2021

albertteoh Jun 17, 2021

yurishkuro Jun 19, 2021

albertteoh Jun 20, 2021

yurishkuro Jun 20, 2021

albertteoh Jun 17, 2021

albertteoh left a comment

yurishkuro Jun 19, 2021

albertteoh Jun 20, 2021

albertteoh Jun 20, 2021 •

edited

Loading

albertteoh Jun 20, 2021

yurishkuro Jun 20, 2021

yurishkuro Jun 20, 2021

yurishkuro Jun 20, 2021

albertteoh Jun 21, 2021

albertteoh Jun 21, 2021

albertteoh Jun 21, 2021

albertteoh Jun 21, 2021

albertteoh commented Jun 21, 2021

Add HTTP handler for metrics querying #3095

Add HTTP handler for metrics querying #3095

Conversation

albertteoh commented Jun 16, 2021

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jun 16, 2021 • 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

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

albertteoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh Jun 20, 2021 • 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

albertteoh commented Jun 21, 2021

codecov bot commented Jun 16, 2021 •

edited

Loading

albertteoh Jun 20, 2021 •

edited

Loading