[query] Add pickled format for graphite render endpoint #1446

arnikola · 2019-03-12T23:50:29Z

No description provided.

codecov · 2019-03-13T00:42:48Z

Codecov Report

Merging #1446 into master will increase coverage by <.1%.
The diff coverage is 47.8%.

@@           Coverage Diff            @@
##           master   #1446     +/-   ##
========================================
+ Coverage    70.8%   70.8%   +<.1%     
========================================
  Files         836     837      +1     
  Lines       71507   71606     +99     
========================================
+ Hits        50676   50748     +72     
- Misses      17532   17546     +14     
- Partials     3299    3312     +13

Flag	Coverage Δ
#aggregator	`82.3% <ø> (ø)`	⬆️
#cluster	`85.8% <ø> (ø)`	⬆️
#collector	`63.7% <ø> (ø)`	⬆️
#dbnode	`80.8% <ø> (ø)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.2% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (-0.2%)`	⬇️
#query	`65.7% <47.8%> (-0.2%)`	⬇️
#x	`76% <ø> (ø)`	⬆️

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 3f4f16e...e58c632. Read the comment docs.

codecov · 2019-03-13T00:42:59Z

Codecov Report

Merging #1446 into master will decrease coverage by <.1%.
The diff coverage is 48.7%.

@@           Coverage Diff            @@
##           master   #1446     +/-   ##
========================================
- Coverage    70.8%   70.8%   -0.1%     
========================================
  Files         836     837      +1     
  Lines       71507   71609    +102     
========================================
+ Hits        50676   50709     +33     
- Misses      17532   17577     +45     
- Partials     3299    3323     +24

Flag	Coverage Δ
#aggregator	`82.3% <ø> (-0.1%)`	⬇️
#cluster	`85.8% <ø> (ø)`	⬆️
#collector	`63.7% <ø> (ø)`	⬆️
#dbnode	`80.6% <ø> (-0.1%)`	⬇️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.2% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (-0.2%)`	⬇️
#query	`65.7% <48.7%> (-0.2%)`	⬇️
#x	`76% <ø> (ø)`	⬆️

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 3f4f16e...e57545f. Read the comment docs.

benraskin92 · 2019-03-14T02:07:31Z

src/query/api/v1/handler/graphite/pickle/pickle_writer.go

+
+// NewWriter creates a new pickle writer.
+func NewWriter(w io.Writer) *Writer {
+	// TODO(mmihic): Consider pooling


benraskin92 · 2019-03-14T14:22:09Z

src/query/api/v1/handler/graphite/pickle/pickle_writer_test.go

+	"math"
+	"testing"
+
+	"github.com/hydrogen18/stalecucumber"


Make sure this license is okay to use: https://github.com/hydrogen18/stalecucumber/blob/master/LICENSE

Good point; I think that's more permissive than apache2 but will double check

Explicitly called out as legit here: http://www.apache.org/legal/resolved.html#category-a

benraskin92 · 2019-03-14T14:23:39Z

src/query/api/v1/handler/graphite/render_parser.go

+		for i := 0; i < s.Len(); i++ {
+			pw.WriteFloat64(s.ValueAt(i))
+		}
+		pw.EndList()


Do we need:

pw.WriteDictKey("graphOptions") pw.BeginDict() for k, v := range s.GraphOptions { pw.WriteDictKey(k) pw.WriteString(v) } pw.EndDict()

Got rid of this since we don't take graph options as an arg; it's things for display options like color etc

benraskin92 · 2019-03-14T14:26:09Z

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

@@ -103,7 +103,7 @@ func NewHandler(
 	embeddedDbCfg *dbconfig.DBConfiguration,
 	scope tally.Scope,
 ) (*Handler, error) {
-	r := mux.NewRouter()
+	r := mux.NewRouter().StrictSlash(true)


We want to redirect?

Also, guess other people are confused... gorilla/mux#145

Oops this was me trying things out and accidentally adding it here

benraskin92 · 2019-03-14T14:57:26Z

src/query/graphite/storage/m3_wrapper.go

-	m3query := translateQuery(query, opts)
+	m3query, err := translateQuery(query, opts)
+	if err != nil {
+		// NB: error here implies the query cannot be translated; empty set expected


I feel like we should just return the error, no? we can discuss offline.

This is mostly for graphite web integration which is happier receiving empty results

Log error instead then?

Added a log

benraskin92 · 2019-03-14T14:58:30Z

src/query/api/v1/handler/graphite/pickle/pickle_writer.go

+
+// A Writer is capable writing out the opcodes required by the pickle format.
+// Note that this is a very limited implementation of pickling; just enough for
+// us to implement the opcodes required by graphite /render


. at the end :)

benraskin92 · 2019-03-14T14:58:55Z

src/query/api/v1/handler/graphite/pickle/pickle_writer.go

+	p.WriteString(s)
+}
+
+// EndDict ends marshalling a python dict


benraskin92 · 2019-03-14T14:59:00Z

src/query/api/v1/handler/graphite/pickle/pickle_writer.go

+	p.err = p.w.WriteByte(opSetItems)
+}
+
+// BeginList begins writing a new python list


benraskin92 · 2019-03-14T14:59:04Z

src/query/api/v1/handler/graphite/pickle/pickle_writer.go

+	_, p.err = p.w.Write(listStart)
+}
+
+// EndList ends writing a python list


benraskin92 · 2019-03-14T14:59:14Z

src/query/api/v1/handler/graphite/pickle/pickle_writer.go

+	p.w.WriteByte(opAppends)
+}
+
+// WriteNone writes a python None


Same with all comments!

benraskin92 · 2019-03-14T15:00:53Z

src/query/api/v1/handler/graphite/pickle/pickle_writer_test.go

+	w.WriteString("hello world")
+	w.WriteDictKey("nested")
+	w.BeginDict()
+	w.WriteDictKey("monkeyFoods")


Can you clean up the actual wording :) monkeyFoods is kind of weird..

Just test data, but changed 👍

benraskin92 · 2019-03-14T21:38:45Z

src/query/api/v1/handler/graphite/find_parser.go

-	matchers := graphiteStorage.TranslateQueryToMatchers(query)
+	matchers, err := graphiteStorage.TranslateQueryToMatchers(query)
+	if err != nil {
+		return nil, xhttp.NewParseError(fmt.Errorf("invalid 'query': %s", query),


Why the quotes? 'query'

It provides some additional context that specifically the query parameter is the issue

benraskin92 · 2019-03-14T21:40:39Z

src/query/api/v1/handler/graphite/pickle/opcodes.go

+
+package pickle
+
+// op list.


Could you add an additional comment explaining what these are used for?

benraskin92 · 2019-03-14T23:11:39Z

src/query/graphite/storage/m3_wrapper.go

@@ -47,18 +48,40 @@ func NewM3WrappedStorage(m3storage storage.Storage) Storage {
 	return &m3WrappedStore{m3: m3storage}
 }

-// TranslateQueryToMatchers converts a graphite query to tag matcher pairs.
-func TranslateQueryToMatchers(query string) models.Matchers {
+// translates a graphite query to tag matcher pairs


benraskin92

LGTM

[query] Add pickled format for graphite render endpoint

7aff74f

arnikola requested review from andrewmains12 and benraskin92 March 12, 2019 23:50

Linter fix

e58c632

benraskin92 reviewed Mar 14, 2019

View reviewed changes

PR response

6a34868

benraskin92 reviewed Mar 14, 2019

View reviewed changes

PR response

e57545f

benraskin92 approved these changes Mar 15, 2019

View reviewed changes

arnikola and others added 2 commits March 15, 2019 14:12

Merge branch 'master' into arnikola/pickle-format

dfb9b54

Merge issue

51d0a3d

arnikola merged commit 55d8b41 into master Mar 15, 2019

justinjc deleted the arnikola/pickle-format branch June 17, 2019 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Add pickled format for graphite render endpoint #1446

[query] Add pickled format for graphite render endpoint #1446

arnikola commented Mar 12, 2019

codecov bot commented Mar 13, 2019

codecov bot commented Mar 13, 2019 •

edited

Loading

benraskin92 Mar 14, 2019

benraskin92 Mar 14, 2019

arnikola Mar 14, 2019

arnikola Mar 14, 2019

benraskin92 Mar 14, 2019

arnikola Mar 14, 2019

benraskin92 Mar 14, 2019

arnikola Mar 14, 2019

benraskin92 Mar 14, 2019

arnikola Mar 14, 2019

benraskin92 Mar 14, 2019

arnikola Mar 15, 2019

benraskin92 Mar 14, 2019

benraskin92 Mar 14, 2019

benraskin92 Mar 14, 2019

benraskin92 Mar 14, 2019

benraskin92 Mar 14, 2019

benraskin92 Mar 14, 2019

arnikola Mar 15, 2019

benraskin92 Mar 14, 2019

arnikola Mar 15, 2019

benraskin92 Mar 14, 2019

benraskin92 Mar 14, 2019

benraskin92 left a comment

[query] Add pickled format for graphite render endpoint #1446

[query] Add pickled format for graphite render endpoint #1446

Conversation

arnikola commented Mar 12, 2019

codecov bot commented Mar 13, 2019

Codecov Report

codecov bot commented Mar 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benraskin92 left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 13, 2019 •

edited

Loading