-
Notifications
You must be signed in to change notification settings - Fork 326
Don't record measurements with no subscription #600
Conversation
We can actually easily use the ref count. |
c0bdcc4
to
a37ffec
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.
nice! left a suggestion on how i think this might be made even cheaper but also happy if you want to get this in now and address that later
stats/record.go
Outdated
// Record records one or multiple measurements with the same tags at once. | ||
// If there are any tags in the context, measurements will be tagged with them. | ||
func Record(ctx context.Context, ms ...Measurement) { | ||
if len(ms) == 0 { | ||
var filtered []Measurement |
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 don't think it is worth it to construct a filtered slice here. I think we can make this even cheaper by:
- Instead of having a subscribed call per measure, have
M
construct aMeasure
with nilm
and define this to mean that the Measurement is ignored - Just do a pass over the
ms
and check if all are ignored and drop if that's the case, otherwise pass the originalms
- In the view code, filter the Measurements by
Measure() != nil
when processing view updates.
This would avoid allocations in append
and also virtual calls to subscribed
.
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.
Done
@@ -57,6 +58,7 @@ func (cmd *subscribeToViewReq) handleCommand(w *worker) { | |||
errstr = append(errstr, fmt.Sprintf("%s: %v", view.Name, err)) | |||
continue | |||
} | |||
internal.SubscriptionReporter(view.Measure.Name()) |
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.
may as well just pass Measure and call the .subscribe directly
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 can't, it creates a circular dependency.
@@ -22,10 +22,24 @@ import ( | |||
"go.opencensus.io/tag" | |||
) | |||
|
|||
func init() { | |||
internal.SubscriptionReporter = func(measure string) { |
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.
may as well accept Measure and set internal.SubscriptionReporter = measure.subscribed
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 can't, it creates a circular dependency.
2ef573f
to
8c3b385
Compare
Suggested by Ramon, immediately dropping the measurements from the measures with no subscription rather than doing it at a later time significantly improves the performance on the critical path. Before: BenchmarkRecord0-8 500000000 3.09 ns/op BenchmarkRecord1-8 5000000 366 ns/op BenchmarkRecord8-8 3000000 412 ns/op BenchmarkRecord8_Parallel-8 2000000 804 ns/op BenchmarkRecord8_8Tags-8 3000000 415 ns/op After: BenchmarkRecord0-8 1000000000 2.58 ns/op BenchmarkRecord1-8 30000000 36.9 ns/op BenchmarkRecord8-8 20000000 89.4 ns/op BenchmarkRecord8_Parallel-8 30000000 44.8 ns/op BenchmarkRecord8_8Tags-8 20000000 90.1 ns/op
Suggested by Ramon, immediately dropping the measurements from
the measures with no subscription rather than doing it at a later time
significantly improves the performance on the critical path.
Before:
BenchmarkRecord0-8 500000000 3.09 ns/op
BenchmarkRecord1-8 5000000 366 ns/op
BenchmarkRecord8-8 3000000 412 ns/op
BenchmarkRecord8_Parallel-8 2000000 804 ns/op
BenchmarkRecord8_8Tags-8 3000000 415 ns/op
After:
BenchmarkRecord0-8 500000000 3.75 ns/op
BenchmarkRecord1-8 30000000 39.7 ns/op
BenchmarkRecord8-8 20000000 101 ns/op
BenchmarkRecord8_Parallel-8 30000000 40.1 ns/op
BenchmarkRecord8_8Tags-8 20000000 102 ns/op