Importer with bigtable support #1291

replay · 2019-04-24T18:09:05Z

Modifies the whisper-importer-reader and writer utilities so they:

use chunk write requests to send the data from the reader to the writer, which makes it easier to plug any store into the writer that satisfies the mdata.Store interface
support bigtable as a store and as an index, both can be chosen separately

Unfortunately some of the arguments that need to be passed to the writer had to be changed, so the invocation will need to be updated (HM-API).

It requires some of the data structures in github.com/raintank/schema to be msgp marshalable, which they currently aren't. Once this PR is accepted I'll create an according PR there.

This is not tested yet.

This already includes the PR #1290, once that one is merged this diff will be shorter

woodsaj · 2019-04-25T08:02:21Z

cmd/mt-whisper-importer-reader/conversion.go

-		adjustedPoints["sum"] = make(map[uint32]float64)
+	adjustedPoints := make(map[schema.Method]map[uint32]float64)
+	if retIdx > 0 && c.method == schema.Avg || c.method == schema.Sum {
+		adjustedPoints[schema.Cnt] = make(map[uint32]float64)


why do we keep a count aggregation if all the user wants is 'sum'?

I think that was because that way we can also serve average queries

we should stop doing that. The "count" value we are storing will be wrong any time the original metrics stream had null points. eg, if the raw interval is 1min and their is a 10min SUM rollup, we cant assume that the sum is the result of 10 points. It could be the result of anywhere from 0-10 points.

But maybe we should address this problem in a followup PR.

mdata/aggmetric.go

mdata/cwr.go

store/bigtable/bigtable.go

store/cassandra/cassandra.go

store/bigtable/bigtable.go

mdata/cwr.go

cmd/mt-whisper-importer-reader/main.go

woodsaj · 2019-04-26T11:00:50Z

mdata/cwr.go

+		return errors.New(fmt.Sprintf("ERROR: Creating Gzip reader: %q", err))
+	}
+
+	raw, err := ioutil.ReadAll(gzipReader)


There is no need to load the uncompressed data into memory, msgp can decode from a io.Reader directly.
eg,

err := msgp.Decode(gzipReader, a)

mdata/cwr.go

replay · 2019-04-26T14:49:15Z

Thx for all the comments @woodsaj , will fix everything. What do you think about that I replaced the CLI flags with the same configuration method like what MT is using in 84217de? I figured that way it's going to be much easier to configure it from HM-API, because the majority of the store/index configs are just going to be the same as in the MT write deployments.

woodsaj · 2019-04-26T17:30:43Z

What do you think about that I replaced the CLI flags with the same configuration method like what MT is using

I love it.

woodsaj · 2019-04-26T18:25:42Z

mdata/cwr.go

+	versionBuf := make([]byte, 1)
+	readBytes, err := b.Read(versionBuf)
+	if err != nil || readBytes != 1 {
+		return errors.New(fmt.Sprintf("ERROR: Failed to read one byte: %s", err))


instead of errors.New(fmt.Sprintf(... you can just use fmt.Errorf(

replay · 2019-05-08T23:46:41Z

I first redeployed my test instance in the QA cluster and made it use BigTable as index & store, then I imported 5 test metrics from generated whisper files, after restarting the read MT they showed up correctly.
Then I redeployed the instance and made it use Cassandra and imported the same test metrics into Cassandra, after restarting the read MT they again showed up correctly.

Dieterbe · 2019-05-09T09:13:26Z

just fix my last comment,
and also why is anything changing in the vendor dir?
seems that in one place we removed an import for github.com/raintank/schema , andthen elsewhere introduced it again, so why are we making changes to it?

replay · 2019-05-09T14:17:12Z

@Dieterbe regarding the changes in the vendor dir. because now we need the schema.AMKey and its member the schema.Archive to be marshallable (if that's a verb). I mentioned that in the top message: #1291 (comment)
I'll create a PR to raintank/schema as soon as we're sure that this is the final solution that we want.

Dieterbe · 2019-05-09T14:43:02Z

oh! well, when i came up with schema.Archive, MKey, Key, etc my idea was that these would all be internal implementation details that could be changed at any time because their exact values would only stay inside of a process, and never be serialized and sent in network requests.

For serialisation I think think we should use the string representation of the keys (e.g. 'orgid.deadbeef_sum_60'). this makes it easier to inspect them on the wire too

replay · 2019-05-09T14:56:07Z

For serialisation I think think we should use the string representation of the keys (e.g. 'orgid.deadbeef_sum_60').

That would mean we couldn't use mdata.ChunkWriteRequest anymore. We'd have to build some kind of struct which is basically the same, with the only difference being that it uses a string for the key. I thought one of the "nice" things in this PR is that it makes everything very short and concise, because we can directly transmit the cwrs, so we'd then lose that benefit.

Dieterbe · 2019-05-09T15:17:19Z

That would mean we couldn't use mdata.ChunkWriteRequest anymore. We'd have to build some kind of struct which is basically the same, with the only difference being that it uses a string for the key.

you're conflating a struct and its serialized representation.
you can totally use mdata.ChunkWriteRequest, but when serializing and deserializing, use the string representation of the AMKey. this may involve a tweak to AMKey to use its string representation in the msgp methods, i'm not sure what's the concrete way to do it.

replay · 2019-05-09T15:36:56Z

@Dieterbe i think you're right, I'll try that

replay · 2019-05-09T20:42:35Z

@Dieterbe how's this: 39123b1
Or would you prefer if I try to find a way to implement the same thing without modifying raintank/schema at all? I think this would probably be possible by writing a similar extension for the ChunkWriteRequest type, it would just be a bit more complicated than this solution and I figured maybe somebody else also wants to encode the AMKey as string.

Dieterbe · 2019-05-10T05:59:21Z

looks good, but there's 2 things left in schema that still need to be cleaned up:
Archive should not need any msgp code, and neither should Point, as far as i can see.

is it correct to say the only change in the schema library should be that we add msgp encoding to the AMKey (by serializing it to its string representation) ?

Also, the commit history has lots of back and forth, can we "compact" the commits (via git rebase -i)
it should ultimately be 2 or 3 commits:

the switch to using CWR for the importer, and all its corresponding changes in tests etc
the change in how we handle config management
(updates to docs/tools.md though that could be part of the 1/2 as well)

I suggest give it a shot, and see how far it can be cleaned up (i tend to do many runs of git rebase -i, each time squashing some commits together, until it becomes clear that i'll hit a merge conflict, or if i hit a merge conflict during rebasing i then just do git rebase --abort. if you do many smaller runs that still leaves you with the cleaned up history from previous runs)

replay · 2019-05-12T02:25:26Z

is it correct to say the only change in the schema library should be that we add msgp encoding to the AMKey (by serializing it to its string representation) ?

I also fixed a typo in an error message and msgp generated a bunch of tests for that extension, otherwise that's right. If this PR is accepted I can create a PR to raintank/schema with those changes. There I can create separate commits for the typo and the msgp stuff. Then, once that's merged, we can just update the vendored raintank/schema to the latest master and there shouldn't be any code changes only the revision hash should change.

I fixed up the git history. Usually I do that by just squashing all changes into one commit, then I go back and edit that commit and use either git add -p or the + in vscode to add change by change and create separate commits that way (basically splitting it).
I made sure that even the first one of those two commits, without the second one, compiles. But it won't be useful, because it's not configurable. Only both together are useful.

Dieterbe · 2019-05-12T11:17:53Z

so why does your PR still introduce a bunch of other files like point_gen_test.go and metric_gen_test.go ?
once we're only making the changes to schema that we need, and nothing else, this is all good to go
( the change to method_string.go is fine, that's due to a new stringer version)

when transferring the data between the whisper-importer-reader and -writer we now use the same chunk write request type that metrictank uses internally when submitting writes to its stores. this makes it easier to add other store types to the importer-writer if we add more to metrictank in the future. futhermore, it modifies the chunk write requests so they include a callback to be called when a chunk has been written. this helps to decouple the mdata package from the store package by letting the instantiator of the chunk write request adding any generic callback to it.

this makes the configuration of the importer-writer consistent with the way how MT and other MT-tools are configured. this makes sense to do now because with the added support for the bigtable store and index the configuration would get too complex if we'd try to come up with a way to configure all of that via cli arguments, so its easier and more consistent to just do it in the same way like everything else.

replay · 2019-05-13T16:02:21Z

@Dieterbe I cleaned it up, please take another look
I previously regenerated the files by doing go generate vendor/github.com/raintank/schema/*.go, which also generated the tests, but usually those don't get vendored. I removed the tests now.

replay · 2019-05-16T22:54:36Z

I found a critical bug and fixed it and added a test that covers it:
0461e94#diff-2f8bddc453c2ba697972cbeff0844d5fL86

it took me forever to find that, i only saw that some metrics looked weird because certain chunks which were supposed to be archive chunks overwrote some raw chunks, but not always....

after fixing it i did some more test imports with cassandra and bigtable backends, and it all looks fine

Dieterbe · 2019-05-17T08:34:55Z

yep please patch in schema so we can merge there, introduce here cleanly, and merge here.

replay changed the title ~~[WIP] Importer bigtable~~ [WIP] Importer with bigtable support Apr 24, 2019

replay force-pushed the importer_bigtable branch 3 times, most recently from 39f8294 to bb424c2 Compare April 24, 2019 20:41