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

Prepare to remove LabelSet #539

Closed
jmacd opened this issue Mar 11, 2020 · 0 comments · Fixed by #574
Closed

Prepare to remove LabelSet #539

jmacd opened this issue Mar 11, 2020 · 0 comments · Fixed by #574
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics

Comments

@jmacd
Copy link
Contributor

jmacd commented Mar 11, 2020

There are three parts to this ticket to prepare the SDK for removing LabelSet from the API, as specified in open-telemetry/oteps#90.

  1. As mentioned here, there is a TODO about replacing the []core.KeyValue slice by a Get(int) core.KeyValue interface so that the array representation can support the interface, meaning we do not need to hold a reference to the original kvs ...core.KeyValue argument (which could be modified by the caller).
  2. As mentioned here, following PR Use a variable-size array to represent ordered labels in maps #523 the label encoding will be recomputed once per collection interval per label set, which is a regression that we'd like fixed.
  3. There is a great (and bad) dependency created between the SDK and the Batcher (Integrator) and the Exporters where the LabelEncoder is concerned. This was an optimization, as shown in Use a variable-size array to represent ordered labels in maps #523, since the cost of computing a single label encoding once is less than the cost of one array map key and one single label encoding. However, the code complexity is not worth it, we think, and after Use a variable-size array to represent ordered labels in maps #523 the label encoding for a given set of labels is only needed for the Batcher (Integrator) and Exporters, not the SDK (Accumulator) itself. If more than one label encoding are needed by more than one exporter, the current code structure causes trouble.

In OTEP 78, specifically here or really here, the LabelSet was made concrete and allowed to cache more than one label encoding. This was because OTEP 78 was moving LabelSet into the API. To address OTEP 90, we will remove the Meter Labels(...) API, but first we should address the three points above.

This can be done by re-implementing the sdk/meter.labels type, currently defined

	labels struct {
		meter   *SDK
		sorted  sortedLabels
		encoded string
	}

As we will remove Labels(...), this struct can mostly shift into the record itself. The record will now need a reference to its SDK. The sortedLabels is replaced in #523 by orderedLabels which will remain private to the SDK. The export.Record type will store an interface (of the *record) that supports Get(int) core.KeyValue instead of exposing a slice directly, which addresses this comment. and the TODO in (1) above. The export.Record, in the same interface as Get(int) core.KeyValue will also support Encode(LabelEncoder) string, as shown in #427. Lastly, the Encode() function will support caching a small fixed number of encodings. This will be stored directly in the record, since these cached values should persist across collection intervals.

These steps will make it easy to remove LabelSet from the user-facing API in a future PR. 🚀

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 a pull request may close this issue.

2 participants