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

Merge Int64DataPoint and DoubleDataPoint into ScalarDataPoint and support different sum types in Histogram #208

Closed
wants to merge 1 commit into from

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Aug 21, 2020

This is the first option to fix #206

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

@bogdandrutu bogdandrutu requested review from a team August 21, 2020 17:22
@bogdandrutu bogdandrutu force-pushed the scalardp branch 2 times, most recently from b4e83af to b69e538 Compare August 21, 2020 17:39
@bogdandrutu bogdandrutu changed the title Merge Int64DataPoint and DoubleDataPoint into ScalarDataPoint Merge Int64DataPoint and DoubleDataPoint into ScalarDataPoint and support different sum types in Histogram Aug 21, 2020
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 prefer #209 as it uses less memory, but I also support this option as it tends to produce smaller code.

Copy link
Member

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

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

I would value the readability & maintainability of this proto going forward (which might allow faster iteration & increase adoption) more highly than a <1% performance difference in this case, so prefer this over #209. I find easier to read this than the other one, and I would imagine the other approach would get messier (more duplication) as we add more to it.

(but I don't feel that strongly about it either way)

@jmacd
Copy link
Contributor

jmacd commented Aug 25, 2020

@jkwatson put in a vote for this PR in Gitter. I'm on the fence, but I should mention that I proposed a similar PR to this one in the past: #172

I would prefer that this is settled than to debate these options, though. Can we move forward with this version?

@tigrannajaryan
Copy link
Member

I agree performance difference is negligible and we need to choose based on usability or some other criteria.
@bogdandrutu can you also assign the value to measurement_value_type where appropriate to make the code complete?

@bogdandrutu
Copy link
Member Author

Thought about this more and I prefer #209 for the reasons I put in gitter: 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.

It seems there is no clear winner so I would want to move forward with that one

@bogdandrutu
Copy link
Member Author

I am going to close this and ask everyone to review #209.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide if for Gauge/Sum use measurement type or different types IntGauge/DoubleGauge
4 participants