Add m3collector service and config with report endpoint #1050

robskillington · 2018-10-09T08:03:23Z

Tests

richardartoul · 2018-10-09T18:33:11Z

src/collector/api/v1/handler/json/report.go

+	"net/http"
+
+	"github.com/m3db/m3/src/collector/reporter"
+	"github.com/m3db/m3/src/query/api/v1/handler"


query dependencies feel a little weird....probably not much we can do about models / storage right now but if you're only importing the error function I wonder if its worth just duplicating it

ah you're using the WriteJSONResponse too...whatever lol

Yeah, I'll put a todo to clean this up.

Hm, screw it, I'll move it into x/net/http.

richardartoul · 2018-10-09T18:34:00Z

src/collector/api/v1/handler/json/report.go

+
+func (h *reportHandler) parseRequest(r *http.Request) (*reportRequest, *handler.ParseError) {
+	body := r.Body
+	if r.Body == nil {


should you use the variable you just created?

Ah yup, that's kinda strange. Ta.

richardartoul · 2018-10-09T18:40:26Z

src/collector/api/v1/handler/json/report.go

+func (h *reportHandler) newMetricID(metric metricValue) (id.ID, *handler.ParseError) {
+	tags := make(models.Tags, 0, len(metric.Tags))
+	for n, v := range metric.Tags {
+		tags = tags.AddTag(models.Tag{Name: []byte(n), Value: []byte(v)})


I guess the tag name has to be a string after JSON deserialization because you can't have []byte be a map key, but can the JSON map be map[string][]byte so we don't have to convert it here?

Nah unfortunately JSON decoder expects []byte types to be base64 encoded strings, so will make this endpoint very hard to use.

This is a super low volume test endpoint, so don't fret too much about perf here.

richardartoul · 2018-10-09T18:42:48Z

src/collector/api/v1/handler/json/report.go

+	tags = models.Normalize(tags)
+	tagsIter := storage.TagsToIdentTagIterator(tags)
+
+	encoder := h.encoderPool.Get()


do you need a reset here?

Ah good call, cheers.

richardartoul · 2018-10-09T18:47:56Z

src/collector/config/m3collector.yml

+
+  client:
+    placementKV:
+      namespace: /r2/m3aggregator


Is this the default namespace we want to use? Probably dont want r2 in the name

let's just change this to /m3aggregator? @richardartoul need to use the same in coordinator endpoint and m3agg config

Sure thing.

richardartoul · 2018-10-09T18:48:31Z

src/collector/config/m3collector.yml

+    placementKV:
+      namespace: /r2/m3aggregator
+    placementWatcher:
+      key: placement


you're watching for m3agg placements right? I think this will be the name of the service in the placement so make this m3agg maybe?

cw9 · 2018-10-09T18:59:32Z

src/collector/config/m3collector.yml

+  matcher:
+    initWatchTimeout: 10s
+    rulesKVConfig:
+      namespace: /r2/rules


remove /r2 here as well

cw9 · 2018-10-09T19:11:04Z

src/collector/config/m3collector.yml

+
+  client:
+    placementKV:
+      namespace: /r2/m3aggregator


let's just change this to /m3aggregator? @richardartoul need to use the same in coordinator endpoint and m3agg config

codecov · 2018-10-09T22:19:55Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1050   +/-   ##
=======================================
  Coverage   77.48%   77.48%           
=======================================
  Files         535      535           
  Lines       45661    45661           
=======================================
  Hits        35381    35381           
  Misses       8045     8045           
  Partials     2235     2235

Flag	Coverage Δ
#aggregator	`81.65% <0%> (ø)`	⬆️
#collector	`61.88% <0%> (ø)`	⬆️
#dbnode	`81.27% <0%> (ø)`	⬆️
#m3em	`73.21% <0%> (ø)`	⬆️
#m3ninx	`75.25% <0%> (ø)`	⬆️
#m3nsch	`51.19% <0%> (ø)`	⬆️
#query	`63.32% <0%> (ø)`	⬆️
#x	`76.2% <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 8081448...19d68c6. Read the comment docs.

richardartoul · 2018-10-09T22:36:57Z

src/collector/server/server.go

+
+	go func() {
+		logger.Info("starting server", zap.String("address", listenAddress))
+		if err := srv.ListenAndServe(); err != nil {


shouldnt this fatal or something?

Sure thing, I'll make sure err != http.ErrServerClosed too so we don't fatal in the "normal" sigterm case where we explicitly shut it down ourselves (need to retain clean exit).

richardartoul

:stamp:

richardartoul reviewed Oct 9, 2018

View reviewed changes

cw9 reviewed Oct 9, 2018

View reviewed changes

richardartoul reviewed Oct 9, 2018

View reviewed changes

richardartoul approved these changes Oct 9, 2018

View reviewed changes

Rob Skillington added 6 commits October 9, 2018 18:51

Add m3collector service and config with report endpoint

5c230c3

Add tests

6282706

Fix build

9532c80

Address feedback

b132ae1

Fix build errors

20beeb7

Address feedback and generate assets

2f4da96

robskillington force-pushed the r/add-collector-scaffold branch from 4d287e1 to 2f4da96 Compare October 9, 2018 22:51

Move file

f7095aa

robskillington merged commit c37e065 into master Oct 9, 2018

robskillington deleted the r/add-collector-scaffold branch October 9, 2018 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add m3collector service and config with report endpoint #1050

Add m3collector service and config with report endpoint #1050

robskillington commented Oct 9, 2018 •

edited

Loading

richardartoul Oct 9, 2018

richardartoul Oct 9, 2018

robskillington Oct 9, 2018

robskillington Oct 9, 2018 •

edited

Loading

richardartoul Oct 9, 2018

robskillington Oct 9, 2018

richardartoul Oct 9, 2018

robskillington Oct 9, 2018

richardartoul Oct 9, 2018

robskillington Oct 9, 2018

richardartoul Oct 9, 2018

cw9 Oct 9, 2018

robskillington Oct 9, 2018

richardartoul Oct 9, 2018

cw9 Oct 9, 2018

cw9 Oct 9, 2018

codecov bot commented Oct 9, 2018 •

edited

Loading

richardartoul Oct 9, 2018

robskillington Oct 9, 2018

richardartoul left a comment

Add m3collector service and config with report endpoint #1050

Add m3collector service and config with report endpoint #1050

Conversation

robskillington commented Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Oct 9, 2018 • 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

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 Oct 9, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardartoul left a comment

Choose a reason for hiding this comment

robskillington commented Oct 9, 2018 •

edited

Loading

robskillington Oct 9, 2018 •

edited

Loading

codecov bot commented Oct 9, 2018 •

edited

Loading