Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Remove convTslice calls in Record() #1268

Merged
merged 2 commits into from Oct 21, 2021
Merged

Remove convTslice calls in Record() #1268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90a0d3c
Remove `convTslice` calls in `Record()`
howardjohn Sep 14, 2021
afd0e34
Refactor to avoid leaking into public API
howardjohn Sep 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions stats/internal/record.go
View file
Open in desktop
Expand Up @@ -21,5 +21,11 @@ import (
// DefaultRecorder will be called for each Record call.
var DefaultRecorder func(tags *tag.Map, measurement interface{}, attachments map[string]interface{})

// MeasurementRecorder will be called for each Record call. This is the same as DefaultRecorder but
// avoids interface{} conversion.
// This will be a func(tags *tag.Map, measurement []Measurement, attachments map[string]interface{}) type,
// but is interface{} here to avoid import loops
var MeasurementRecorder interface{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone overrode this with a func(tags *tag.Map, measurement []Measurement, attachments map[string]interface{}), would recorder := internal.MeasurementRecorder.(measurementRecorder) accept it?

At first glance, this seems unnecessary, since this is already in an internal package

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure what you mean:

if you mean modified this L28 to use that instead of interface{} it would not compile (import loop)

If you mean set the value; we do (internal.MeasurementRecorder = recordMeasurement) but it can only be done in this package since its in internal, so a user couldn't do it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see the import loop now. This approach is probably the best we can do. Are the benchmarks in the description updated for the latest attempt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated it, but its the same (besides recording noise) as the initial iteration.


// SubscriptionReporter reports when a view subscribed with a measure.
var SubscriptionReporter func(measure string)
4 changes: 3 additions & 1 deletion stats/record.go
View file
Open in desktop
Expand Up @@ -86,6 +86,8 @@ func createRecordOption(ros ...Options) *recordOptions {
return o
}

type measurementRecorder = func(tags *tag.Map, measurement []Measurement, attachments map[string]interface{})

// Record records one or multiple measurements with the same context at once.
// If there are any tags in the context, measurements will be tagged with them.
func Record(ctx context.Context, ms ...Measurement) {
Expand All @@ -94,7 +96,7 @@ func Record(ctx context.Context, ms ...Measurement) {
if len(ms) == 0 {
return
}
recorder := internal.DefaultRecorder
recorder := internal.MeasurementRecorder.(measurementRecorder)
record := false
for _, m := range ms {
if m.desc.subscribed() {
Expand Down
13 changes: 12 additions & 1 deletion stats/view/worker.go
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@ func init() {
defaultWorker = NewMeter().(*worker)
go defaultWorker.start()
internal.DefaultRecorder = record
internal.MeasurementRecorder = recordMeasurement
}

type measureRef struct {
Expand Down Expand Up @@ -199,11 +200,21 @@ func record(tags *tag.Map, ms interface{}, attachments map[string]interface{}) {
defaultWorker.Record(tags, ms, attachments)
}

func recordMeasurement(tags *tag.Map, ms []stats.Measurement, attachments map[string]interface{}) {
defaultWorker.recordMeasurement(tags, ms, attachments)
}

// Record records a set of measurements ms associated with the given tags and attachments.
func (w *worker) Record(tags *tag.Map, ms interface{}, attachments map[string]interface{}) {
w.recordMeasurement(tags, ms.([]stats.Measurement), attachments)
}

// recordMeasurement records a set of measurements ms associated with the given tags and attachments.
// This is the same as Record but without an interface{} type to avoid allocations
func (w *worker) recordMeasurement(tags *tag.Map, ms []stats.Measurement, attachments map[string]interface{}) {
req := &recordReq{
tm: tags,
ms: ms.([]stats.Measurement),
ms: ms,
attachments: attachments,
t: time.Now(),
}
Expand Down