mt-gateway: support ingesting data into kafka #1608

fitzoh · 2020-01-15T19:54:00Z

Initial portion of this PR was porting the github.com/raintank/tsdb-gw/publish package to this repository for use in a new HTTP proxy (see https://github.com/grafana/metrictank-ops/issues/542 if you have access).

The portion after this comment (specifically after the force push) pulls in the actual API endpoint and wires it up to Kafka.

A couple design decisions:

This is intended to be a simplified API, so rate limiting functionality has been removed.
As there is no concept of authentication/authorization within metrictank, the isAdmin=true path is always taken.

Once again, the latter portion of this PR is intended to be reviewed commit by commit.

Open questions:

Do we need to consider any licensing/attribution issues?
- probably fine, could use a +1 from @woodsaj ✔️
Is there any dead code we can get rid of post-import?
- removed ✔️
Should we maintain the instrumentation present in the original code?
- leave it ✔️
Testing? Do we add this to a docker config? Which one?
- toss it in docker-dev-custom-cfg-kafka alongside tsdb-gw for now

publish/publish.go

publish/kafka/schemas.go

replay · 2020-01-17T22:11:41Z

To be sure I understand this diff right, this doesn't change the Sarama version, it only adds another package right? I'm asking because when changing Sarama versions it's usually a good idea to do some benchmarks to verify that there are no regressions.

replay · 2020-01-17T22:13:24Z

And currently none of this code which gets added is actually being used, right? That's all going to be used by your new http gateway?

replay · 2020-01-17T22:16:26Z

By merging the dependencies of the mt-gateway into the MT repo separately before they actually get used, isn't there a risk that we accidentally copy more stuff from tsdb-gw than what's actually going to be used and this will then be left there as dead code, or are you sure that all of this is needed?

robert-milan · 2020-01-17T22:32:35Z

To be sure I understand this diff right, this doesn't change the Sarama version, it only adds another package right? I'm asking because when changing Sarama versions it's usually a good idea to do some benchmarks to verify that there are no regressions.

Yes that is correct. I thought the same thing at first glance, but it only adds mocks.

I understand that breaking it up like this makes for smaller and easier to read PRs, but in this case it does seem a little bit awkward. I think this should be part of the PR that uses this code in the HTTP proxy. Since #1602 is already merged, can we just rename this PR and add the HTTP proxy changes here as well? It is easy enough to review commit by commit.

Do we need to consider any licensing/attribution issues?

We probably should take a look at everything

Are we able to omit any portion of this package?

That would be easier to determine once we see some of the implementation

Should we maintain the instrumentation present in the original?

I don't think we need to maintain the instrumentation unless there is a good reason for it.

fitzoh · 2020-01-17T23:11:36Z

To be sure I understand this diff right, this doesn't change the Sarama version, it only adds another package right? I'm asking because when changing Sarama versions it's usually a good idea to do some benchmarks to verify that there are no regressions.

Yes that is correct. I thought the same thing at first glance, but it only adds mocks.

Correct, @robert-milan

And currently none of this code which gets added is actually being used, right? That's all going to be used by your new http gateway?

yep

I understand that breaking it up like this makes for smaller and easier to read PRs, but in this case it does seem a little bit awkward. I think this should be part of the PR that uses this code in the HTTP proxy. Since #1602 is already merged, can we just rename this PR and add the HTTP proxy changes here as well? It is easy enough to review commit by commit.

Yeah, good call. This was mostly an attempt to unblock myself while #1602 was waiting on reviews... now that it's merged, I'll rebase and work on wiring things up wire things up

…mpile at the moment)

…compile)

…ation errors

…ssues

…rrors

replay · 2020-01-22T15:39:26Z

cmd/mt-gateway/ingest/metrics.go

 		default:
 			log.Errorf("unable to read request body. %s", err)
-			ctx.JSON(500, err)
+			w.WriteHeader(500)
+			fmt.Fprintf(w, "unable to read request body. %s", err)


there's a repeating pattern of writing a status code header, then writing a message on the following line, and in some cases also log a line with the same msg.
would it make sense to put these into a small helper like writeStatus(w, 499, "request cancelled", true) (the last param could determine whether to log).

Yeah, that had crossed my mind.
Skipped the boolean and just always log @ error (successful ingestions take a different code path)

1abc592

cmd/mt-gateway/api.go

replay

Looks good, just a few minor comments

cmd/mt-gateway/ingest/metrics.go

replay

LGTM

fitzoh requested a review from a team January 15, 2020 19:54

fitzoh commented Jan 16, 2020

View reviewed changes

publish/publish.go Outdated Show resolved Hide resolved

fitzoh commented Jan 16, 2020

View reviewed changes

publish/kafka/schemas.go Outdated Show resolved Hide resolved

fitzoh added 10 commits January 21, 2020 02:51

initial import of "publish" package from raintank/tsdb-gw (doesn't co…

f748bf3

…mpile at the moment)

update imports from raintank/tsdb-gw -> grafana/metrictank (still no …

66a9481

…compile)

copy-paste metrics_client package from raintank/tsdb_gw to fix compil…

6b76e24

…ation errors

pull in util/flags package from raintank/tsdb-gw to fix compilation i…

e89cc6e

…ssues

import BufferPool33 struct from raintank/tsdb-gw to fix compilation e…

85e9c4f

…rrors

fix compilation error

d955422

add missing publisher test dependency (github.com/Shopify/sarama/mocks)

e37ea38

go fmt

0b5c3fc

get rid of unused variable and the dependency it pulled in

c2ccac6

remove unnecessary function/file

3f7baea

fitzoh force-pushed the kafka-publish branch from 34f4d38 to 3f7baea Compare January 21, 2020 07:51

fitzoh added 9 commits January 21, 2020 03:41

initial ingest of metrics ingestion files

8674c57

update imports

d50c88e

drop rate limiting

3a270f8

Replace macaron ctx object with base http objects

d4d0b7d

There's no auth in this version, so we should assume admin = true

60a6788

Wire up new ingest handler

7adf01d

go fmt

82e377e

configure kafka for ingestor endpoint

94c2112

sync tool docs

b6887d9

fitzoh changed the title ~~Import publish package and dependencies from raintank/tsdb-gw~~ Add kafka import endpoint + capabilities from raintank/tsdb-gw Jan 21, 2020

fitzoh removed the request for review from a team January 21, 2020 15:07

fitzoh requested a review from a team January 21, 2020 15:07

replay reviewed Jan 22, 2020

View reviewed changes

cmd/mt-gateway/api.go Outdated Show resolved Hide resolved

replay reviewed Jan 22, 2020

View reviewed changes

cmd/mt-gateway/api.go Outdated Show resolved Hide resolved

replay reviewed Jan 22, 2020

View reviewed changes

cmd/mt-gateway/api.go Outdated Show resolved Hide resolved

replay suggested changes Jan 22, 2020

View reviewed changes

fitzoh added 4 commits January 22, 2020 13:05

fix message + make more descriptive

87cb16c

update imports

186fab2

Refactor responses/logging for failed ingest requests

1abc592

remove unused function

c90e7a7

replay reviewed Jan 22, 2020

View reviewed changes

cmd/mt-gateway/ingest/metrics.go Outdated Show resolved Hide resolved

Fprintf -> Fprint

3237276

replay approved these changes Jan 22, 2020

View reviewed changes

Dieterbe changed the title ~~Add kafka import endpoint + capabilities from raintank/tsdb-gw~~ mt-gateway: support ingesting data into kafka Jan 23, 2020

Dieterbe merged commit 9450798 into master Jan 23, 2020

Dieterbe deleted the kafka-publish branch January 23, 2020 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mt-gateway: support ingesting data into kafka #1608

mt-gateway: support ingesting data into kafka #1608

fitzoh commented Jan 15, 2020 •

edited

Loading

replay commented Jan 17, 2020

replay commented Jan 17, 2020

replay commented Jan 17, 2020

robert-milan commented Jan 17, 2020

fitzoh commented Jan 17, 2020 •

edited

Loading

replay Jan 22, 2020

fitzoh Jan 22, 2020

replay left a comment

replay left a comment

mt-gateway: support ingesting data into kafka #1608

mt-gateway: support ingesting data into kafka #1608

Conversation

fitzoh commented Jan 15, 2020 • edited Loading

replay commented Jan 17, 2020

replay commented Jan 17, 2020

replay commented Jan 17, 2020

robert-milan commented Jan 17, 2020

fitzoh commented Jan 17, 2020 • edited Loading

replay Jan 22, 2020

Choose a reason for hiding this comment

fitzoh Jan 22, 2020

Choose a reason for hiding this comment

replay left a comment

Choose a reason for hiding this comment

replay left a comment

Choose a reason for hiding this comment

fitzoh commented Jan 15, 2020 •

edited

Loading

fitzoh commented Jan 17, 2020 •

edited

Loading