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.

Sign up for GitHub

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

Jump to bottom

Unsplit all data points and exemplar by measurement type #209

Merged
merged 1 commit into from
Aug 26, 2020
Merged

Unsplit all data points and exemplar by measurement type #209

merged 1 commit into from
Aug 26, 2020

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Aug 21, 2020 •
edited
Loading

Cannot see any significant difference between merged messages vs split messages, code is here https://github.com/bogdandrutu/metrics-proto

goos: darwin
goarch: amd64
pkg: github.com/bogdandrutu/metrics-proto/benchmark
BenchmarkEncodeDecode
BenchmarkEncodeDecode/merged
BenchmarkEncodeDecode/merged-16         	   34406	     33744 ns/op	   30288 B/op	     523 allocs/op
BenchmarkEncodeDecode/unmerged
BenchmarkEncodeDecode/unmerged-16       	   36130	     33190 ns/op	   29168 B/op	     523 allocs/op

Advantages over #208:

  • extensibility - can add new types that work only on some measurement types like strings if we decide to support that)
  • no corner cases like measurement type is long and exemplars are using double values.

@bogdandrutu bogdandrutu requested review from a team August 21, 2020 17:38
@bogdandrutu bogdandrutu mentioned this pull request Aug 21, 2020
Merged
jmacd
jmacd approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

I support this as it uses slightly less memory than the proposal in #208.

@bogdandrutu bogdandrutu mentioned this pull request Aug 26, 2020
Closed
tigrannajaryan
tigrannajaryan reviewed Aug 26, 2020
View reviewed changes
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
opentelemetry/proto/metrics/v1/metrics.proto Outdated Show resolved Hide resolved
tigrannajaryan
tigrannajaryan approved these changes Aug 26, 2020
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

@bogdandrutu
Copy link
Member Author

Will rebase this with the labels renaming in Exemplars please continue to review

@bogdandrutu
Unsplit all data points and exemplar by measurement type
4cbb7af 
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu bogdandrutu merged commit f374b78 into open-telemetry:master Aug 26, 2020
@bogdandrutu bogdandrutu deleted the unmargeall branch August 26, 2020 23:21
@jsuereth jsuereth mentioned this pull request Apr 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmacd jmacd jmacd approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bogdandrutu @jmacd @tigrannajaryan