-
Notifications
You must be signed in to change notification settings - Fork 107
Conversation
20484f9
to
98f590b
Compare
goals and consequences -> extra sub-goals
it would be prettier to do these steps one at the time but because they're so tangled let's just bite the bullet and do them at once. also helps that we won't have to test again later. statusMaking good progress on this. main things left to do:
what is changing in this PR:
what is NOT changing:
note to self. to test:
deployment
to reviewers
|
idx/cassandra/cassandra.go
Outdated
@@ -365,8 +365,15 @@ func (c *CasIdx) load(defs []schema.MetricDefinition, iter cqlIterator, cutoff u | |||
var tags []string | |||
cutoff64 := int64(cutoff) | |||
for iter.Scan(&id, &orgId, &partition, &name, &metric, &interval, &unit, &mtype, &tags, &lastupdate) { | |||
mkey, err := schema.MKeyFromString(id) | |||
if er != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be err
not er
msg/msg.go
Outdated
// Point represents an incoming datapoint | ||
// represented either as MetricData or MetricPointId2 | ||
type Point struct { | ||
Val uint8 // 0 means use md, 1 means use point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Val
seems like the wrong term to use here. Perhaps we use DataType
and then use
var MetricData = uint8(0)
var MetricPoint = uint8(1)
Which will make the code much more readable
p := msg.Point{
DataType: msg.MetricPoint,
Point: point,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. or const even not var. but i decided to do away with this composite msg.Point type and instead use different codepaths in the input handler and index. will push soon.
98f590b
to
2878d78
Compare
mdata/cache/ccache.go
Outdated
@@ -125,20 +126,20 @@ func (c *CCache) CacheIfHot(metric string, prev uint32, itergen chunk.IterGen) { | |||
met.Add(prev, itergen) | |||
} | |||
|
|||
func (c *CCache) Add(metric, rawMetric string, prev uint32, itergen chunk.IterGen) { | |||
func (c *CCache) Add(metric, rawMetric schema.AMKey, prev uint32, itergen chunk.IterGen) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still require both params metric
and rawMetric
? I think just one schema.AMKey
contains all the information that's needed, because the raw key can be extracted from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right
mdata/cache/ccache.go
Outdated
|
||
// sets of metric keys, indexed by their raw metric keys | ||
metricRawKeys map[string]map[string]struct{} | ||
metricRawKeys map[schema.AMKey]map[schema.AMKey]struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this be optimized into something like map[schema.MKey]map[schema.Archive]
?
2878d78
to
1eec530
Compare
mdata/cache/ccache_metric.go
Outdated
@@ -20,7 +21,7 @@ type CCacheMetric struct { | |||
// the list of chunk time stamps in ascending order | |||
keys []uint32 | |||
|
|||
SuffixLen uint8 | |||
rawKey schema.AMKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't schema.MKey
be sufficient here?
input/kafkamdm/kafkamdm.go
Outdated
md := schema.MetricData{} | ||
_, err := md.UnmarshalMsg(data) | ||
pointMsg := msg.Point{} | ||
if len(data) == 29 && data[0] == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the unmarshaling of bytes to a msg.Point should be handled in the msg
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved it
mdata/notifierKafka/notifierKafka.go
Outdated
@@ -232,7 +234,13 @@ func (c *NotifierKafka) flush() { | |||
payload := make([]*sarama.ProducerMessage, 0, len(c.buf)) | |||
var pMsg mdata.PersistMessageBatch | |||
for i, msg := range c.buf { | |||
def, ok := c.idx.Get(strings.SplitN(msg.Key, "_", 2)[0]) | |||
mkey, err := schema.MKeyFromString(strings.SplitN(msg.Key, "_", 2)[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't msg.Key be an AMKey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msg.Key is SavedChunk.Key which is a string because I don't want to change the notifier format at this point. it does represent a schema.AMKey (see SavedChunk definition)
if your point is that it's a bit confusing that we're parsing out an MKey here, then i agree. I actually attempted to write an AMKeyFromString function but skipped it for now. maybe i can give it another shot.
but anyway we only need an MKey for the index lookup so doing the string.Split and then parsing that seemed ok to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cleaned this up today.
@@ -32,6 +32,91 @@ type PartitionedMetric interface { | |||
|
|||
//go:generate msgp | |||
|
|||
type MetricPointId1 struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dont think we need two MetricPointId types. If we do, they need meaningful names.
Looking over this PR, it looks like the metricPointId1 is only used for sending over the wire. Currently if the payload received by MT does not have an Org, it is being set to the runtime orgId
config setting.
So instead of 2 struct types, we just need the 2 message formats for the 1 MetricPoint struct.
MarshalWithOrg()
UnmarshalWithOrg()
MarshalWithoutOrg()
UnmarshalWithoutOrg()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dont think we need two MetricPointId types. If we do, they need meaningful names.
i agree. i wanted to use the name MetricPoint but you used that in your pr so i had to separate them at first so i could compare the different implementations.
as i said in OP they are codenames that will be renamed.
i like your idea of switching to a single type though, that will make the code easier.
also while we're at it, I want to use the MKey type in this struct.
72d66d1
to
ec13cb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall these changes look promising. There is just the one concern I have and it seems centered around having deviated from the prescribed key hashing.
idx/cassandra/cassandra.go
Outdated
// abeit very unlikely, | ||
// the idx entry could be pruned in between the two calls and so they could be different | ||
existing, inMemory := c.MemoryIdx.Get(point.MKey) | ||
archive, inMemory2 := c.MemoryIdx.UpdateMaybe(point, partition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that UpdateMaybe
could return existing
as well? That would prevent the unlikely disagreement scenario in the comment (since it would operate under a lock) plus it would prevent an additional read lock from being acquired (which likely increases the area of contention, since it blocks writers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that UpdateMaybe could return existing as well?
it does, but after it updates the LastUpdate and Partition fields.
that said, the only thing we really need existing
for is the existing.Partition attribute which determines if we have to clean up the old entry. UpdateMaybe could just return that value. Not very elegant but could be worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I meant existing
as it is in this snippet (i.e. the value before update). If you don't foresee another need to pop up other than the partition value, then that would likely be the most efficient solution and could greatly reduce the contention on this mutex (conjecture).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll give it a shot :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is done now btw
@@ -91,3 +92,42 @@ func CreateMsg(metrics []*schema.MetricData, id int64, version Format) ([]byte, | |||
} | |||
return buf.Bytes(), nil | |||
} | |||
|
|||
// WritePointMsg is like CreateMsg, except optimized for MetricPoint and buffer re-use. | |||
// caller must be assure a cap-len diff of at least: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"must assure"
idx/cassandra/cassandra.go
Outdated
archive := c.MemoryIdx.AddOrUpdate(data, partition) | ||
|
||
// note that both functions return an 'ok' bool. | ||
// abeit very unlikely, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"albeit"
return bts[28:], nil | ||
} | ||
|
||
// MMarshal32 marshals the MetricPoint directly to b. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Marshal32"
idx/cassandra/cassandra.go
Outdated
} | ||
|
||
if inMemory2 { | ||
archive = c.updateCassandraIfStale(inMemory, archive, partition) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a possibility that updating via just the MetricPoint
could starve out changes that would be seen by the MetricData
?
One example we have is that we don't include the Interval
in the id we generate (for reasons I can explain if you are curious).
This works as-is because we always send the new interval and after a few hours, it gets updated in cassandra and everything is fine (provided the interval was reduced, otherwise the history is a little funky). After this change, it seems that likely the interval change wouldn't be detected unless we send full MetricData
payloads for hours after an interval change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One example we have is that we don't include the Interval in the id we generate (for reasons I can explain if you are curious).
always :)
I think I don't really understand the question/problem,
but there's 2 places where this logic is called (UpdateMaybe which takes the MetricPoint, and AddOrUpdate which takes a full MetricData). however, by the time we get to the updateCassandraIfStale call, it doesn't really matter which of the 2 paths we took to get to this logic, as the calls use the same parameters which it pulled out of the memory index (possibly after an update), and the partition value.
In that regard, whether you sent a MetricData (with the same interval) or a MetricPoint, the logic should be equivalent.
and the ordering should remain honored throughout the kafka->CassandraIndex->Cassandra pipeline.
does this help @shanson7 ? otherwise let's discuss further
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original comment can be ignored. As long as any data that is not encoded into the Id
is in the MetricPoint, then this should be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is one field from MetricData that is not included in MetricPoint and doesn't contribute to its Id, which is Name
. However, since we always say that field should equal the Metric
field (which does contribute the id), this should be OK. Note that because of this, we plan to remove one of these two as they should always be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I noticed that. I use Name
when encoding and set Metric
to .
for everything (to reduce the size of the messages)
note: since the idx structures are mostly already by (or could be restructured to be by) orgid we could potentially use Keys instead of MKey's for e.g IdSet, and shard defById by orgid first. that would save some memory. future optimization, not going to bother with it now I think. also updating orgid to uint32 . |
99fa6f7
to
20b3cd5
Compare
making progress. have been able to do some useful tests https://snapshot.raintank.io/dashboard/snapshot/ZS91Vehxxsd3xp5RTtXeyPu5txOhdbaJ?orgId=2 some preliminary notes:
|
to compare ram and GC effects, here's a fresh stack start and 10 minute long 25kHz ingest, comparing old MT with old payloads, vs new MT with new payloads conclusion:
|
blocked on #260 |
158b3e5
to
8a321e5
Compare
status update:
TODO
|
this helps clarify some things, like why table metric_16 is created
github.com/jpillora/backoff no longer needed due to mt-replicator-via-tsdb removal. others should have been cleaned up longer ago. Not sure why scripts/qa/vendor.sh started complaining about this only now, but anyway.
it's a better name, and frees up the name 'Update'
so we have latest functions available like groupByNodes
6e90517
to
3e02874
Compare
* received and invalid on a per-messagetype basis * track unknown metricpoint metrics * remove unused MsgsAge metric note: dashboard now requires recent graphite which has groupByNodes
751ae46
to
d23d807
Compare
before: https://snapshot.raintank.io/dashboard/snapshot/kGy0wuc5I8QTs67fofENgxTUt4avSCaW?orgId=2 https://snapshot.raintank.io/dashboard/snapshot/wMHOrRhktilGFMTwEUp6ejIkBdWmHv63?orgId=2 after: https://snapshot.raintank.io/dashboard/snapshot/wMHOrRhktilGFMTwEUp6ejIkBdWmHv63?orgId=2 https://snapshot.raintank.io/dashboard/snapshot/QK06sLfjTK6J53TEoJcYd9MY9oZlZFJA?orgId=2 looks like a minor but useful change
rebased on top of master, now we have all the org id stuff and related idx fixes ===> time for another/last round of reviews ! and then let's merge and deploy ! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
see #199
adds support for the following payloads incoming from kafka:
(codenames, will be changed later if we take this further)
(in the actual messages they are preceded by a 1byte format header)
because we now encode the id's in a more optimized form, i will also attempt to update the codebase itself to work with these id's instead of strings; in particular: