Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Labelset #595

Merged
merged 24 commits into from
Mar 27, 2020
Merged

Remove Labelset #595

merged 24 commits into from
Mar 27, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Mar 25, 2020

This addresses #590.

Introduces a static helper function metric.Labels(...core.KeyValue) []core.KeyValue which simplifies rewriting of e.g., Bind(meter.Labels(keys...)) to Bind(metric.Labels(keys...)...). The diff is pretty readable. The only significant changes are in sdk.go where the code is reorganized to avoid a new allocation. Whereas before the labels object was managed as a pointer, one that had to be allocated inside Labels(), labels is now a struct embedded in either type of instrument, and the internal makeLabels() function allocates a temporary record that will become garbage if the lookup succeeds and will become the new record otherwise.

It does not change the RecordBatch API as suggested in OTEP 90 (and I'm not sure that it really matters, after some consideration). This PR does change a lot of benchmarks, but the benchmarks weren't measuring the labelset creation so it's apples and oranges.

We have argued that the cost of label set construction can be amortized using bound handles, which is true. Note that there is not a way to amortize this across collection intervals for Observer instruments, because there is no "bound" form of observer instrument and now no LabelSet to perform that function. This is acceptable; perhaps if there is a dire need we can add bound Observers somehow in the future.

The new RecordBatch benchmark is a good summary of the performance change, BEFORE:

BenchmarkBatchRecord8Labels_1Instrument-8     	 1336000	       889 ns/op	     480 B/op	       2 allocs/op
BenchmarkBatchRecord_8Labels_2Instruments-8   	  786096	      1568 ns/op	     480 B/op	       2 allocs/op
BenchmarkBatchRecord_8Labels_4Instruments-8   	  390052	      2818 ns/op	     480 B/op	       2 allocs/op
BenchmarkBatchRecord_8Labels_8Instruments-8   	  228703	      4937 ns/op	     480 B/op	       2 allocs/op

AFTER:

BenchmarkBatchRecord8Labels_1Instrument-8     	 1250874	       942 ns/op	     512 B/op	       2 allocs/op
BenchmarkBatchRecord_8Labels_2Instruments-8   	  651992	      1626 ns/op	     512 B/op	       2 allocs/op
BenchmarkBatchRecord_8Labels_4Instruments-8   	  396933	      2797 ns/op	     512 B/op	       2 allocs/op
BenchmarkBatchRecord_8Labels_8Instruments-8   	  213150	      5165 ns/op	     512 B/op	       2 allocs/op

@jmacd jmacd added the area:metrics Part of OpenTelemetry Metrics label Mar 25, 2020
@jmacd
Copy link
Contributor Author

jmacd commented Mar 25, 2020

The test failures are related to 32bit builds. Investigating..

@jmacd
Copy link
Contributor Author

jmacd commented Mar 26, 2020

Unfortunately this is a real stress_test failure in GOARCH=386. I've studied this and it points to a troubling conclusion somehow related to a dormant problem with #468. I can't find a change in behavior in the SDK itself, and the problem is easily repeatable on GOARCH=386. The problem has not been reproduced for a 64-bit build, and cannot be reproduced before this PR. I suspect this could relate to 64-bit atomic operations being especially slow on a 32-bit platform. I do not believe this is a case of the (infamous) https://golang.org/issue/599.

@jmacd
Copy link
Contributor Author

jmacd commented Mar 27, 2020

Now the test failure looks unrelated.

    batch_span_processor_test.go:163: parallel span generation: number of exported span: got 205, want 200

Copy link
Member

@krnowak krnowak left a comment

Choose a reason for hiding this comment

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

Rambled a bit about the record reuse. But the PR looks good!

sdk/metric/sdk.go Outdated Show resolved Hide resolved
sdk/metric/sdk.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Looks good. Nothing blocking.

api/metric/api.go Outdated Show resolved Hide resolved
sdk/metric/sdk.go Show resolved Hide resolved
sdk/metric/sdk.go Show resolved Hide resolved
@jmacd jmacd merged commit e8546e3 into open-telemetry:master Mar 27, 2020
@jmacd jmacd deleted the jmacd/remove_labelset branch March 27, 2020 21:06
@jmacd jmacd mentioned this pull request Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants