[query] add list tags endpoint #1565

arnikola · 2019-04-17T16:30:33Z

Adds this Prometheus endpoint that lists all available tags

codecov · 2019-04-17T16:44:21Z

Codecov Report

Merging #1565 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1565   +/-   ##
======================================
  Coverage    71.9%   71.9%           
======================================
  Files         950     950           
  Lines       78800   78800           
======================================
  Hits        56666   56666           
  Misses      18431   18431           
  Partials     3703    3703

Flag	Coverage Δ
#aggregator	`82.3% <0%> (ø)`	⬆️
#cluster	`85.7% <0%> (ø)`	⬆️
#collector	`63.7% <0%> (ø)`	⬆️
#dbnode	`79.9% <0%> (ø)`	⬆️
#m3em	`73.2% <0%> (ø)`	⬆️
#m3ninx	`74% <0%> (ø)`	⬆️
#m3nsch	`51.1% <0%> (ø)`	⬆️
#metrics	`17.6% <0%> (ø)`	⬆️
#msg	`74.7% <0%> (ø)`	⬆️
#query	`66.9% <0%> (ø)`	⬆️
#x	`86.8% <0%> (ø)`	⬆️

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 120fe60...efe5ed9. Read the comment docs.

prateek · 2019-04-18T17:51:45Z

src/query/models/matcher.go

@@ -38,6 +38,8 @@ func (m MatchType) String() string {
 		return "=~"
 	case MatchNotRegexp:
 		return "!~"
+	case MatchAll:
+		return "*"


this String() method is only used for debugging yea?

prateek · 2019-04-18T17:52:18Z

src/query/api/v1/handler/prometheus/common.go

@@ -304,6 +304,35 @@ func renderDefaultTagCompletionResultsJSON(
 	return jw.Close()
 }

+// RenderListTagResultsJSON renders list tag results to json format.
+func RenderListTagResultsJSON(


can we rely on json.Marshal with the appropriate struct instead of defining our own?

Yeah, it's an issue with a bunch of our stuff; called out removing json.Marshal in #1590

Added a response on that issue, for any endpoints where we return a lot of data we should continue using our JSON writers:

One thing interesting about the JSON writers, the major reason in the past we did that is because the default JSON marshaller allocates a lot, however if we have JSON write methods for bytes/strings/whatever other concrete types we can avoid allocations when returning responses (which is what we have today with the current common library streamjson writer).

Yeah was going to write some quick benchmarks when it came time to consider swapping it out; I think our initial thinking here was if we're using this writer internally there's a good reason

prateek · 2019-04-18T17:53:12Z

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

+		return
+	}
+
+	// TODO: Support multiple result types


what do you mean?

Copied an existing handler; removed this note, this should only support json

prateek · 2019-04-18T17:54:06Z

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

+	logger := logging.WithContext(ctx)
+	w.Header().Set("Content-Type", "application/json")
+
+	query := &storage.CompleteTagsQuery{


do you want to add the ability to restrict to only a given set of tag field names?

This is the implementation of the Prom endpoint here, which expects no args and returns all valid tag names only: https://prometheus.io/docs/prometheus/latest/querying/api/#getting-label-names

prateek · 2019-04-18T17:54:15Z

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

+	w.Header().Set("Content-Type", "application/json")
+
+	query := &storage.CompleteTagsQuery{
+		CompleteNameOnly: true,


more a question: what is this for?

This restricts search results to only give tag names rather than tag names and values

prateek · 2019-04-18T17:54:39Z

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

+		TagMatchers:      models.Matchers{{Type: models.MatchAll}},
+
+		// NB: necessarily spans entire possible query range.
+		Start: time.Time{},


this is fine for default but should it be overridable from the incoming request?

Generally yes, but this particular endpoint is intended to work across all time

prateek · 2019-04-18T17:55:13Z

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

+		End:   time.Now(),
+	}
+
+	opts := storage.NewFetchOptions()


this might be a silly question: what are these options/how come we can just use the defaults?

Default is fine here; for context, this includes:

tracing options (could potentially be added)

fetch limit (irrelevant for this endpoint)

block fetch type (irrelevant for this endpoint - this is used to switch between decompressing blocks as soon as fetched vs keeping fetches as series iterators until applying non-lazy functions)

fanout options (this may change in future if we allow a standalone index to use instead of doing our best guess fanout at the moment, but defaults are fine here for virtually all cases; this is mostly here to allow us to override the graphite query path)

prateek · 2019-04-18T17:56:08Z

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

+	Data   []string `json:"data"`
+}
+
+func TestFind(t *testing.T) {


if possible could you add a failure test situation too

Sure sounds good

prateek · 2019-04-18T17:56:56Z

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

+}
+
+func TestFind(t *testing.T) {
+	logging.InitWithCores(nil)


been trying to push the standard stuff we have for zap now, could you use src/x/test/NewLogger to create the test logger and pass down to the appropriate place

Called out in #1590 as well; all our other handlers are doing this at the moment, may be nicer to fix them all at once?

I think we're currently using zap for logging; although we may do it in a bit of a weird way with a global zap logger rather than passing it down

Added this to #1590 under "General":

Remove use of global Zap logger usage in favor of src/x/instrument logger which uses Zap and use src/x/test/NewLogger in tests

benraskin92 · 2019-04-29T17:20:29Z

src/query/api/v1/handler/prometheus/common.go

@@ -304,6 +304,35 @@ func renderDefaultTagCompletionResultsJSON(
 	return jw.Close()
 }

+// RenderListTagResultsJSON renders list tag results to json format.
+func RenderListTagResultsJSON(


Also, is there a scenario where status isn't success?

No; if it errors, it won't use this render function but rather the default error printer

What's the point of having a status field then lol

It's expected in the Prom output unfortunately

benraskin92 · 2019-04-29T17:22:28Z

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

@@ -0,0 +1,90 @@
+// Copyright (c) 2018 Uber Technologies, Inc.


nit: 2019

benraskin92 · 2019-04-29T17:23:19Z

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

+
+const (
+	// ListTagsURL is the url for listing tags.
+	ListTagsURL = handler.RoutePrefixV1 + "/labels"


nit: name ListLabelsURL (here and throughout) to be consistent with Prom?

I'd rather be consistent internally here; we've been renaming prom label endpoints as tags

benraskin92 · 2019-04-29T17:29:53Z

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

+
+func (h *ListTagsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	ctx := context.WithValue(r.Context(), handler.HeaderKey, r.Header)
+	logger := logging.WithContext(ctx)


This using the new logging (zap) stuff?

Should be, same as the other handlers afaik

robskillington · 2019-04-29T19:03:59Z

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

+
+	// execute the query
+	w := &writer{}
+	req := &http.Request{


nit: Better to use httptest.NewRequest(...) I think here?

Yeah this seems much nicer 👍

robskillington · 2019-04-29T19:05:50Z

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

+	handler := NewListTagsHandler(store)
+
+	// execute the query
+	w := &writer{}


nit: Better to use httptest.NewRecorder(...) here:
https://play.golang.org/p/y_M3RA0YUDB

And:
https://golang.org/pkg/net/http/httptest/

robskillington · 2019-04-29T19:06:20Z

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

+		URL: &url.URL{
+			RawQuery: "",
+		},
+	}


Same notes about using httptest.NewRecorder(...) and httptest.NewRequest(...) here.

robskillington

LGTM

[query] add list tags endpoint

ee5f565

arnikola requested review from prateek, andrewmains12 and benraskin92 April 17, 2019 16:30

Use correct handler

e3e001d

arnikola mentioned this pull request Apr 17, 2019

Multi coordinator case improvements #1519

Closed

13 tasks

prateek reviewed Apr 18, 2019

View reviewed changes

benraskin92 reviewed Apr 29, 2019

View reviewed changes

arnikola added 2 commits April 29, 2019 13:46

PR responses

fd7fe63

Merge branch 'master' into arnikola/list_all_tags

09c47e2

robskillington reviewed Apr 29, 2019

View reviewed changes

arnikola and others added 2 commits April 29, 2019 16:14

Refactor test to use httptest.NewRequest()

efe5ed9

Merge branch 'master' into arnikola/list_all_tags

559fff0

robskillington approved these changes May 2, 2019

View reviewed changes

arnikola merged commit e50a400 into master May 2, 2019

arnikola deleted the arnikola/list_all_tags branch May 2, 2019 17:11

arnikola mentioned this pull request May 6, 2019

[query] Ignore name tags for matching binary functions #1604

Merged

		@@ -0,0 +1,90 @@
		// Copyright (c) 2018 Uber Technologies, Inc.

[query] add list tags endpoint #1565

[query] add list tags endpoint #1565

Conversation

arnikola commented Apr 17, 2019 • edited Loading

codecov bot commented Apr 17, 2019 • 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

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

arnikola Apr 29, 2019 • 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

benraskin92 Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

robskillington Apr 29, 2019 • 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

arnikola commented Apr 17, 2019 •

edited

Loading

codecov bot commented Apr 17, 2019 •

edited

Loading

arnikola Apr 29, 2019 •

edited

Loading

benraskin92 Apr 29, 2019 •

edited

Loading

arnikola Apr 29, 2019 •

edited

Loading

robskillington Apr 29, 2019 •

edited

Loading