[dbnode] Emit aggregate usage metrics #3123
Conversation
Also makes aggregate docs limit less aggressive
| @@ -1933,6 +1947,12 @@ func TestBlockAggregate(t *testing.T) { | |||
| spans := mtr.FinishedSpans() | |||
| require.Len(t, spans, 2) | |||
| require.Equal(t, tracepoint.BlockAggregate, spans[0].OperationName) | |||
|
|
|||
| for _, v := range scope.Snapshot().Counters() { | |||
There was a problem hiding this comment.
can use tallytest.AssertCounterValue
There was a problem hiding this comment.
👍 nice, did not know about this one
| func (*noopCounter) Inc(_ int64) {} | ||
|
|
||
| func newUsageMetrics(ns ident.ID, iOpts instrument.Options) usageMetrics { | ||
| if ns == nil { |
There was a problem hiding this comment.
hmm can we have a catch-all metric tag for these instead?
There was a problem hiding this comment.
I was thinking that, but figured that potentially having a bunch of metrics under namespace="undefined" would end up kinda confusing and not add much additional information; can add it if you think it would be worth having though 👍
There was a problem hiding this comment.
gotcha, when is ns not set? Mostly curious if we want to account for these or not
There was a problem hiding this comment.
Mostly when we clear out or finalize the results, or if incorrectly initializing when taking an AggregateResults out of the pool, also happened a lot in tests so had to have some sensible defaults for it without updating all the tests.
There was a problem hiding this comment.
i'd rather not drop metrics. I don't think an undefined namespace is that confusing. it would allow you to sum across the metrics without namespace, which is probably the common case.
| dedupedFields tally.Counter | ||
| } | ||
|
|
||
| func newUsageMetrics(ns ident.ID, iOpts instrument.Options) usageMetrics { |
There was a problem hiding this comment.
do we need to reset this with every aggregate result? Is the namespace important enough?
Codecov Report
@@ Coverage Diff @@
## master #3123 +/- ##
=========================================
- Coverage 72.2% 72.2% -0.1%
=========================================
Files 1084 1084
Lines 100219 100251 +32
=========================================
+ Hits 72397 72420 +23
- Misses 22774 22781 +7
- Partials 5048 5050 +2
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| func (*noopCounter) Inc(_ int64) {} | ||
|
|
||
| func newUsageMetrics(ns ident.ID, iOpts instrument.Options) usageMetrics { | ||
| if ns == nil { |
There was a problem hiding this comment.
i'd rather not drop metrics. I don't think an undefined namespace is that confusing. it would allow you to sum across the metrics without namespace, which is probably the common case.
| // will have one field. | ||
| totalCount := len(batch) | ||
| for idx := 0; idx < len(batch); idx++ { | ||
| totalCount += len(batch[idx].Terms) |
There was a problem hiding this comment.
how is total different than total terms?
* master: [dbnode] Add aggregate term limit regression test (#3135) [DOCS] Adding Prometheus steps to quickstart (#3043) [dbnode] Revert AggregateQuery changes (#3133) Fix TestSessionFetchIDs flaky test (#3132) [dbnode] Alter multi-segments builder to order by size before processing (#3128) [dbnode] Emit aggregate usage metrics (#3123) [dbnode] Add Shard.OpenStreamingReader method (#3119) [dtests] Docker tests integration with docker-compose (#3031) [dbnode] Comments / remove unused var (#3124) [query] Handle context.Canceled and map to 499 http status (#3069) [dbnode] Use StreamingReadMetadata for bootstrapping (#2938) [dbnode] Use DefaultTestOptions in test code (#3113) # Conflicts: # src/dbnode/storage/bootstrap/bootstrapper/fs/source.go
* master: [dtest] endpoint to fetch tagged (#3138) Refactor FetchTagged to return an Iterator of results (#3141) [dbnode] Add aggregate term limit regression test (#3135) [DOCS] Adding Prometheus steps to quickstart (#3043) [dbnode] Revert AggregateQuery changes (#3133) Fix TestSessionFetchIDs flaky test (#3132) [dbnode] Alter multi-segments builder to order by size before processing (#3128) [dbnode] Emit aggregate usage metrics (#3123) [dbnode] Add Shard.OpenStreamingReader method (#3119)
Emits additional aggregate metrics around fields and terms used; also reduces change that made aggregate query doc limit too aggressive