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

Is performance important enough for the MetricDescriptor Type to encode it as a unit64? #189

Closed
bogdandrutu opened this issue Jul 30, 2020 · 8 comments · Fixed by #202
Closed
Assignees
Labels
priority:p1 release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics

Comments

@bogdandrutu
Copy link
Member

Currently there is a TODO in the metrics proto that needs a decision:

// TODO: Determine if encoding all possible values in a uint64 bitset improves performance significantly and propose that if that is the case.

@bogdandrutu bogdandrutu self-assigned this Jul 30, 2020
@bogdandrutu bogdandrutu added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 30, 2020
@jmacd
Copy link
Contributor

jmacd commented Jul 30, 2020

We have discussed that the performance implications of this are more important in the collector. Clients can cache descriptor objects and/or use alternate encoders fairly easily.

@tigrannajaryan
Copy link
Member

It is very likely that there will be significant performance gains for single datapoint metrics. Benchmarking using https://github.com/tigrannajaryan/exp-otelproto should be easy.

@tigrannajaryan
Copy link
Member

Clients can cache descriptor objects and/or use alternate encoders fairly easily.

True, but I suspect most clients won't bother, so would be good not to rely on this. Using bitmasks is trivial in most (?) languages, so I don't think usability-wise it is any worse than oneof with a bunch of messages (in fact my preference would be a single bitfield with masks regardless of performance).

@jmacd
Copy link
Contributor

jmacd commented Jul 30, 2020

(In #168 I wrote a program to generate the bits of all valid combinations...)

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Jul 31, 2020

@tigrannajaryan are you most worried about bytes on the wire or number of allocations?

The biggest problem with PR #168 was that it makes things harder to use since not every possible combination in a bitmap is valid.

@bogdandrutu
Copy link
Member Author

As an example the current Gauge does not have temporality and we tried to add a new temporality just for that, then found hard to reason about all the corner cases

@tigrannajaryan
Copy link
Member

@tigrannajaryan are you most worried about bytes on the wire or number of allocations?

The later.

The biggest problem with PR #168 was that it makes things harder to use since not every possible combination in a bitmap is valid.

Can we provide valid combinations of bitmasks as enums/consts and clearly document that other combinations are not valid?

@bogdandrutu
Copy link
Member Author

The later.

Let me give a try to use gogo proto to avoid allocations. If that works we should not worry and complicate things.

bogdandrutu added a commit to bogdandrutu/opentelemetry-proto that referenced this issue Aug 5, 2020
bogdandrutu added a commit that referenced this issue Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p1 release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants