Remove convTslice calls in Record()

[howardjohn]

This is built upon #1267; that
one should likely merge first. I split this out as it has a small public
API change (to work around circular imports) to avoid issues on the
first PR.

Benchmark relative to #1267:

name                    old time/op    new time/op    delta
Record0-6                 1.77ns ± 1%    1.59ns ± 3%   -9.98%  (p=0.008 n=5+5)
Record1-6                  593ns ± 8%     544ns ± 8%   -8.40%  (p=0.095 n=5+5)
Record8-6                 1.20µs ± 3%    1.23µs ± 3%   +2.34%  (p=0.206 n=5+5)
Record8_WithRecorder-6     765ns ± 7%     783ns ± 6%     ~     (p=0.690 n=5+5)
Record8_Parallel-6        1.19µs ± 2%    1.19µs ± 6%     ~     (p=0.548 n=5+5)
Record8_8Tags-6           1.22µs ± 3%    1.22µs ± 3%     ~     (p=1.000 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  0.00B          0.00B          ~     (all equal)
Record1-6                   120B ± 0%       96B ± 0%  -20.00%  (p=0.008 n=5+5)
Record8-6                   344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%     ~     (all equal)
Record8_Parallel-6          344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_8Tags-6             344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   0.00           0.00          ~     (all equal)
Record1-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%     ~     (all equal)
Record8_Parallel-6          3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_8Tags-6             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)

For #1265

[dashpole]

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.

[howardjohn]

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

[howardjohn]

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.

[dashpole]

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

[howardjohn]

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?

[dashpole]

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?

[howardjohn]

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

[dashpole]

I'm going to hold off merging this for a day or two to make sure others have a chance to comment.

[howardjohn]

Is this good to go?

[dashpole]

cc @punya
for a second opinion

[punya]

Sorry for the very late review. I thought of another approach for handling the import cycle (moving Measurement into the internal package and using a type alias), but it seems like a lot more code change for not enough value. I'm 👍 for this PR.