Feat/buffered data metric#31
Conversation
c60ec8c to
5f06f99
Compare
| record.checkpointer.update(aws.ToString(record.record.SequenceNumber)) | ||
| } | ||
| atomic.AddInt64(&k.recordsInMemoryCount, -1) | ||
| atomic.AddInt64(&k.bytesInMemoryCount, -record.payloadBytes) |
There was a problem hiding this comment.
Do we need an atomic process or can we just throw these values into channels that are then consumed out of the critical path? Worried this will add quite a bit of locking to what is currently a multi-threaded processing loop right?
There was a problem hiding this comment.
Oh I think I follow that suggestion - yeah that's possibly better will have a go at that design
There was a problem hiding this comment.
OK, pushed a refactor that I think resolves this, but keeps channel usage overhead to a minimum as well. LMK what you think!
| // RecordsInMemoryBytes implementation that writes to statsd a gauge metric about | ||
| // the current total payload bytes buffered in memory | ||
| func (s *Statsd) RecordsInMemoryBytes(bytes int64) { | ||
| _ = s.client.Gauge("kinsumer.records_in_memory_bytes", bytes, 1.0) |
There was a problem hiding this comment.
If you want these exported out to cloudwatch the metrics need to be prefixed with "snowplow." - not sure how we are currently dealing with the kinsumer metrics today?
There was a problem hiding this comment.
We currently don't configure it, so we don't get kinsumer metrics.
I'll take a look into it, might need to add a prefix to the config, thanks for flagging. Edit: we just pass the same statsd client through to kinsumer, and we'll get the prefixed metrics :)
On this note - if I enable all of these into statsd, do they all automatically get sent through to cloudwatch etc?
Or is it a case of us having downstream control of what we funnel through to the customer infra, to manage costs?
There was a problem hiding this comment.
They would all go through - there is no filtering in container monitor
There was a problem hiding this comment.
If you want to be cost conscious you would need to have a code-level control to say which metrics to send out (or not)
e2562b9 to
65d6668
Compare
7f01690 to
d9e3a13
Compare
65d6668 to
75e8722
Compare
* Make metrics config cleaner * go fmt
|
I've merged two PRs into this one, each reviewed individually. To tweak impementation, add a test, and add a config for filtering metrics out (and then a PR to clean up that config with a much nicer pattern). Since everything's reviewed, and these changes shouldn't be critical to functionality, I don't think it's necessary to put people through the pain of re-reviewing this one. It will all be reviewed again in final release PR. Assuming no CI testing issues, I'll squash and merge into release branch |
* Initial metrics implementation for tracking record in memory * Buffer metrics before sending * Fix metrics to account for pre-buffer in-memory data * Add a metric which measures memory backlog * Use int64 everywhere for recordsInMemory metric * Use channel instead of atomics to avoid possible contention * Log warnings if we drop metrics * Combine the updates to reduce channel traffic overhead * Reduce channel activity/overhead with a batching mechanic * Use a smaller channel and log immediately if blocked * Buffer metrics every second * Add test of new metrics implementation * Add config to filter metrics * Make metrics config cleaner (#35) * Make metrics config cleaner * go fmt
3 participants
No description provided.