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 1 commit
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
5 changes: 4 additions & 1 deletion stats/record.go
View file
Open in desktop
Expand Up @@ -86,6 +86,9 @@ func createRecordOption(ros ...Options) *recordOptions {
return o
}

// InternalMeasurementRecorder is a recorder for internal usage only.
var InternalMeasurementRecorder 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 +97,7 @@ func Record(ctx context.Context, ms ...Measurement) {
if len(ms) == 0 {
return
}
recorder := internal.DefaultRecorder
recorder := InternalMeasurementRecorder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you help me double-check my reasoning here? In theory, this could break someone who was overriding internal.DefaultRecorder, but that isn't possible, since it is in an internal package.

As long as this is backwards-compatible, I think we can accept the change.

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 agree with your assessment; its backwards compatible because its internal and thus cannot be used. However, it does add a new public field that a user could start messing with when they probably shouldn't. I could not find a way to avoid this without introducing an import loop unfortunately.

edit: I may have an idea, let me try it out

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 refactored it to avoid leaking into the public API, and its still backwards compatible since they could not override internal.DefaultRecorder as mentioned above.

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
stats.InternalMeasurementRecorder = 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