*: Add initial remote-write-receive component by brancz · Pull Request #811 · thanos-io/thanos

brancz · 2019-02-05T14:43:30Z

Changes

This adds the very first pieces of the proposal to add receiving remote-write support to Thanos. The features are just having a single tsdb that remote write requests are written into and that the component joins a mesh network to enable querying. All other features are still outstanding but can be worked on in parallel once this lands.

Verification

As this is very small and primarily hooks up existing components, the extend of my verification is that I added this new component to the quickstart script and verified that indeed data replicated to the endpoint receiving remote-write calls can be queried via the Thanos querier. Happy to add this to the e2e test suite though.

@bwplotka @domgreen

cc @metalmatze @mxinden @squat @s-urbaniak @ant31

bwplotka · 2019-02-05T15:19:28Z

Finally <3

First of all, as you can see our CI does not like NOT checked errors (: and both:

fmt.Fprintf	fmt.Fprintf(w, "Service Unavailable")
(*net/http.Server).Shutdown	httpSrv.Shutdown(ctx)

looks like it would be nice at least log them.

brancz · 2019-02-05T17:00:15Z

Looks like that made CI happy 🙂 .

brancz · 2019-02-05T17:00:57Z

I think having a simple e2e test added would be good. I'll look into adding one.

bwplotka

Generally LGTM, just super small nits, mostly around style. (:

Nice work!

bwplotka · 2019-02-06T01:34:50Z

cmd/thanos/remote-write-receive.go

+)
+
+func registerRemoteWriteReceive(m map[string]setupFunc, app *kingpin.Application, name string) {
+	cmd := app.Command(name, "accept Prometheus remote write API requests (EXPERIMENTAL, this may change drastically without notice)")


Suggested change

cmd := app.Command(name, "accept Prometheus remote write API requests (EXPERIMENTAL, this may change drastically without notice)")

cmd := app.Command(name, "Accept Prometheus remote write API requests (EXPERIMENTAL, this may change drastically without notice)")

bwplotka · 2019-02-06T01:40:27Z

cmd/thanos/remote-write-receive.go

+				return nil
+			},
+			func(err error) {
+				level.Debug(logger).Log("msg", "initial load group errored", "err", err)


hmm, I don't get this log messages here and on every group. This code is invoked even though this group did not errof (but e.g other group errored). I think we can remove those debug logs. We will print error for problematic group via g.Run anyway.

Sorry, this is a leftover from some debugging, I'll remove it.

bwplotka · 2019-02-06T01:41:20Z

cmd/thanos/remote-write-receive.go

+			},
+			func(err error) {
+				level.Debug(logger).Log("msg", "tsdb group errored", "err", err)
+				if err := localStorage.Close(); err != nil {


just checking .. localStorage.Close closes db as well?

bwplotka · 2019-02-06T01:41:38Z

cmd/thanos/remote-write-receive.go

+			func(err error) {
+				level.Debug(logger).Log("msg", "tsdb group errored", "err", err)
+				if err := localStorage.Close(); err != nil {
+					level.Error(logger).Log("msg", "Error stopping storage", "err", err)


all log lines should start lowercase

bwplotka · 2019-02-06T01:42:47Z

cmd/thanos/remote-write-receive.go

+
+	level.Debug(logger).Log("msg", "setting up metric http listen-group")
+	if err := metricHTTPListenGroup(g, logger, reg, httpMetricsBindAddr); err != nil {
+		level.Debug(logger).Log("msg", "metric listener errored", "err", err)


Replicated error handling. (rule "Handle error just once") (:

I think error wrap is the way to go (and removal of this log line).

bwplotka · 2019-02-06T01:50:58Z

pkg/receive/handler.go

+
+	var (
+		m     = cmux.New(listener)
+		httpl = m.Match(cmux.HTTP1Fast())


Can you comment what this is for? It might be not straightforward.

bwplotka · 2019-02-06T01:51:48Z

pkg/receive/handler.go

+		return
+	}
+
+	err = h.receiver.Receive(&wreq)


we can do if err := ... ; err != nil as well

bwplotka · 2019-02-06T01:52:06Z

pkg/receive/receivewriter.go

+package receive
+
+import (
+	"github.com/go-kit/kit/log"


make format needed (:

bwplotka · 2019-02-06T01:53:17Z

scripts/prometheus.yml

@@ -0,0 +1,10 @@
+# When the Thanos remote-write-receive component is started,


Hm.. can we add this to remote-write-receive command description instead? Might be easier to discover

bwplotka · 2019-02-06T01:54:10Z

scripts/quickstart.sh


+sleep 0.5
+
+if [ -n "${REMOTE_WRITE_ENABLED}" ]


Wow I did not know that this script still works (:

bwplotka · 2019-02-06T01:55:46Z

And yea, some tests would be nice. (:

metalmatze · 2019-02-06T13:19:13Z

cmd/thanos/remote-write-receive.go

+
+	var metadata = &promMetadata{
+		// Start out with the full time range. The shipper will constrain it later.
+		// TODO(fabxc): minimum timestamp is never adjusted if shipping is disabled.


Was this code copied from somewhere? Slightly confusing to see TODO(fabxc) in here, but not a big deal I guess.

haha yea was surprised as well at the beginning (: It is copied indeed.

metalmatze · 2019-02-06T13:51:44Z

pkg/receive/handler.go

+// Options for the web Handler.
+type Options struct {
+	Receiver       *ReceiveWriter
+	Context        context.Context


This can be removed as it is not used.

metalmatze

Only a few minor things found next to what @bwplotka already discovered.
Overall really good stuff! 👍

metalmatze · 2019-02-06T13:51:58Z

pkg/receive/handler.go

+// Handler serves various HTTP endpoints of the Prometheus server
+type Handler struct {
+	readyStorage *tsdb.ReadyStorage
+	context      context.Context


This can be removed as it is not used.

metalmatze · 2019-02-06T13:54:34Z

pkg/receive/handler.go

+		ReadTimeout: h.options.ReadTimeout,
+	}
+
+	errCh := make(chan error)


Yes, this should be a run.Group.

metalmatze · 2019-02-06T13:55:27Z

pkg/receive/handler.go

+func (h *Handler) receive(w http.ResponseWriter, req *http.Request) {
+	defer func() {
+		if r := recover(); r != nil {
+			fmt.Println("panic recovered:", r, string(debug.Stack()))


Maybe we want to have a counter for how many panics were recovered?

metalmatze · 2019-02-06T13:57:22Z

pkg/receive/receivewriter.go

+	Appender() (storage.Appender, error)
+}
+
+type ReceiveWriter struct {


This is spelled receive.ReceiveWriter and should be renamed to receive.Writer to be more idiomatic.

brancz · 2019-03-06T16:35:43Z

@bwplotka @metalmatze I believe I addressed everything or answered the comments and added the receiver to the e2e test setup (the test is that a prometheus is successfully pushing an up metric of a "down" node-exporter the the remote-write-receiver).

domgreen

👍 Looks solid overall just a few nits. 👯‍♂️

Sorry again for taking so long to get around to this :(

domgreen · 2019-03-11T09:42:27Z

cmd/thanos/main.go

 	registerCompact(cmds, app, "compact")
 	registerBucket(cmds, app, "bucket")
 	registerDownsample(cmds, app, "downsample")
+	registerRemoteWriteReceive(cmds, app, "remote-write-receive")


nit - would shorten this to just receive/remote /receiver

domgreen · 2019-03-11T09:44:19Z

cmd/thanos/remote-write-receive.go

+			cancel()
+			peer.Close(2 * time.Second)
+		})
+	}


can we remove this whole block? Will receive need to peer via gossip? If we are removing gossip in the near future I think we should not add it to any new components and instead help people migrate to the new approach.

domgreen · 2019-03-11T09:46:09Z

cmd/thanos/remote-write-receive.go

+	level.Debug(logger).Log("msg", "setting up grpc server")
+	{
+		var (
+			logger = log.With(logger, "component", "receiver")


receiver > receive? or the verbose name? Let's keep it consistant.

domgreen · 2019-03-11T09:47:28Z

pkg/component/component.go

+	Rule        = sourceStoreAPI{component: component{name: "rule"}}
+	Sidecar     = sourceStoreAPI{component: component{name: "sidecar"}}
+	Store       = sourceStoreAPI{component: component{name: "store"}}
+	RemoteWrite = sourceStoreAPI{component: component{name: "remote_write"}}


consistency

domgreen · 2019-03-11T09:49:15Z

pkg/receive/handler.go

+
+	reqBuf, err := snappy.Decode(nil, compressed)
+	if err != nil {
+		fmt.Println("snappy decode error")


fmt > logger?

domgreen · 2019-03-11T09:50:46Z

scripts/quickstart.sh

+
+if [ -n "${REMOTE_WRITE_ENABLED}" ]
+then
+  ./thanos remote-write-receive \


👍 this is great taht were adding it here as well.

either we leave this or remove gossip 🙂

@bwplotka what do you think on this ... personally, I think we should phase out gossip so (in a different PR) change this file to work via the new approach.
So for this PR I would remove receive from this and once the above work has been done add it back in.

As we have the e2e tests, that works for me.

As there is no git-history of this, I'm going to comment this out for now, but leave it. Does that sound like an ok compromise?

I'm happy with that ... can you make sure a comment about why its commented is included 😄

Sounds good to me

domgreen · 2019-03-11T09:52:28Z

cmd/thanos/remote-write-receive.go

+	dataDir string,
+	peer cluster.Peer,
+) error {
+	level.Info(logger).Log("msg", "setting up receiver")


Is it worth adding a warn log here saying that it is currently experimental?

very good idea 👍

bwplotka · 2019-03-11T13:42:47Z

pkg/receive/handler.go

+var (
+	requestDuration = prometheus.NewHistogramVec(
+		prometheus.HistogramOpts{
+			Name:    "prometheus_http_request_duration_seconds",


We might want thanos prefix for those metrics? Or do we want to keep it similar to get advantage of common dashboards?

Sorry that was a copy paste mistake. Yes lets make it thanos_http_....

bwplotka · 2019-03-11T13:43:18Z

pkg/receive/handler.go

+	return func(w http.ResponseWriter, r *http.Request) {
+		if h.isReady() {
+			f(w, r)
+		} else {


just return here, no need for else

bwplotka · 2019-03-11T13:43:32Z

pkg/receive/handler.go

+		ReadTimeout: h.options.ReadTimeout,
+	}
+
+	errCh := make(chan error)


still valid?

bwplotka

Still some unaddressed bit, plus some suggestions.. Overall LGTM, but bit major is not versioned remote read receive endpoint, thoughts? (:

IF all will be addressed -> LGTM from me.

bwplotka · 2019-03-11T18:29:53Z

cmd/thanos/receive.go

+		Default("./data").String()
+
+	m[name] = func(g *run.Group, logger log.Logger, reg *prometheus.Registry, tracer opentracing.Tracer, _ bool) error {
+		peer, err := newPeerFn(logger, reg, false, "", false)


You can actualy remove gossip stuff if you want, up to you - making sure deprecated stuff is in new component is quite not necessary. (: Not a blocker

bwplotka · 2019-03-11T18:31:02Z

cmd/thanos/receive.go

+	dataDir string,
+	peer cluster.Peer,
+) error {
+	level.Info(logger).Log("msg", "setting up receive")


Let's kill this message, below as setting up receive; this component is ...

bwplotka · 2019-03-11T18:31:33Z

cmd/thanos/receive.go

+				select {
+				case <-dbOpen:
+					break
+				// In case a shutdown is initiated before the dbOpen is released


Missing trailing period in comment (:

Also I would remove this, I think this is quite clear.

bwplotka · 2019-03-11T18:32:39Z

cmd/thanos/receive.go

+	level.Debug(logger).Log("msg", "setting up grpc server")
+	{
+		var (
+			logger = log.With(logger, "component", "receiver")


Why not doing this in very top?

bwplotka · 2019-03-11T18:33:19Z

go.sum

 github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
 github.com/onsi/gomega v1.4.1 h1:PZSj/UFNaVp3KxrzHOcS7oyuWA7LoOY/77yCTEFu21U=
 github.com/onsi/gomega v1.4.1/go.mod h1:C1qb7wdrVGGVU+Z6iS04AVkA3Q65CEZX59MT0QO5uiA=
+github.com/opentracing-contrib/go-stdlib v0.0.0-20170113013457-1de4cc2120e7 h1:8KbikWulLUcMM96hBxjgoo6gTmCkG6HYSDohv/WygYU=


modules ❤️

bwplotka · 2019-03-11T18:35:26Z

pkg/receive/handler.go

+	}
+
+	readyf := h.testReady
+	router.Post("/receive", readyf(h.receive))


Ok, so this starts to be some API. We already keep Prometheus like API to guide people and be intuitive. This means that remote receiver would be nice to use the same or similar... does Prometheus serves remote write endpoint? What endpoint looks like? api/v1/write ? As a basics something like v1 in path is useful.

tend to agree but flexible on the url ... api/v1/receive feels more correct to me

agreed with the prefix, going with /api/v1/receive, no strong opinion though

bwplotka · 2019-03-11T18:35:54Z

pkg/receive/handler.go

+		ReadTimeout: h.options.ReadTimeout,
+	}
+
+	errCh := make(chan error)


Not addressed

brancz · 2019-03-12T12:58:28Z

I believe I addressed everything:

Added run group handling for the http handler
Removed gossip
Took the freedom to convert the quickstart script from gossip to static --store flag configuration

bwplotka

Awesome, all LGTM from my side.

bwplotka · 2019-03-12T14:27:05Z

cmd/thanos/flags.go

 )

-func regCommonServerFlags(cmd *kingpin.CmdClause) (
+func regGRPCFlags(cmd *kingpin.CmdClause) (


hahah I was starring at monitor 1m and thinking why you removed Gossip for everyone in this PR... then seen that actually you added this function.... Github troll =D

brancz force-pushed the remote-write-receive branch from d042091 to 162c12f Compare February 5, 2019 15:31

bwplotka approved these changes Feb 6, 2019

View reviewed changes

metalmatze reviewed Feb 6, 2019

View reviewed changes

metalmatze suggested changes Feb 6, 2019

View reviewed changes

brancz force-pushed the remote-write-receive branch 6 times, most recently from d7765cc to abf0029 Compare March 6, 2019 16:33

domgreen reviewed Mar 11, 2019

View reviewed changes

brancz force-pushed the remote-write-receive branch from abf0029 to 6c371a6 Compare March 11, 2019 13:41

bwplotka reviewed Mar 11, 2019

View reviewed changes

brancz force-pushed the remote-write-receive branch from 6c371a6 to e1315c8 Compare March 11, 2019 14:19

domgreen approved these changes Mar 11, 2019

View reviewed changes

bwplotka reviewed Mar 11, 2019

View reviewed changes

brancz force-pushed the remote-write-receive branch 8 times, most recently from 1ab4011 to 1a7ea0c Compare March 12, 2019 08:35

brancz force-pushed the remote-write-receive branch 4 times, most recently from 57c0542 to d459673 Compare March 12, 2019 12:30

*: Add initial remote-write-receive component

6cb54d4

brancz force-pushed the remote-write-receive branch from d459673 to 6cb54d4 Compare March 12, 2019 12:45

bwplotka approved these changes Mar 12, 2019

View reviewed changes

bwplotka merged commit b74717a into thanos-io:master Mar 12, 2019

brancz deleted the remote-write-receive branch March 12, 2019 16:26

earthdiaosi pushed a commit to earthdiaosi/thanos that referenced this pull request Mar 14, 2019

*: Add initial remote-write-receive component (thanos-io#811)

3986a59

brancz mentioned this pull request Apr 1, 2019

Prometheus incorrectly handles blocks created by thanos-receiver Go-SIP/thanos-receiver#2

Closed

brancz mentioned this pull request Apr 29, 2019

Thanos receive component umbrella issue #1093

Closed

9 tasks

gillg mentioned this pull request Apr 14, 2020

promtool: Add Data Importers for the most requested formats. prometheus/prometheus#7119

Closed

5 tasks

	cmd := app.Command(name, "accept Prometheus remote write API requests (EXPERIMENTAL, this may change drastically without notice)")
	cmd := app.Command(name, "Accept Prometheus remote write API requests (EXPERIMENTAL, this may change drastically without notice)")

		@@ -0,0 +1,10 @@
		# When the Thanos remote-write-receive component is started,

Conversation

brancz commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Verification

Uh oh!

bwplotka commented Feb 5, 2019

Uh oh!

brancz commented Feb 5, 2019

Uh oh!

brancz commented Feb 5, 2019

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka commented Feb 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metalmatze left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brancz commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domgreen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brancz commented Feb 5, 2019 •

edited

Loading

brancz commented Mar 6, 2019 •

edited

Loading