use gogo proto from jsuereth/protobuf fork for optional support #4259
Conversation
| @@ -3269,6 +3273,19 @@ func (m *HistogramDataPoint) MarshalToSizedBuffer(dAtA []byte) (int, error) { | |||
| return len(dAtA) - i, nil | |||
| } | |||
|
|
|||
| func (m *HistogramDataPoint_Sum) MarshalTo(dAtA []byte) (int, error) { | |||
There was a problem hiding this comment.
Looks like a bug in the upstream gogoproto fork, let me work on fixing that. This should not be generated, instead the above if m.Sum != nil should have been generated.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@codeboten Sorry for long delay, just did a round of bug fixes to gogoproto around optional generation for gogofaster.
|
Codecov Report
@@ Coverage Diff @@
## main #4259 +/- ##
==========================================
+ Coverage 88.07% 90.73% +2.65%
==========================================
Files 173 178 +5
Lines 10188 10360 +172
==========================================
+ Hits 8973 9400 +427
+ Misses 975 743 -232
+ Partials 240 217 -23
Continue to review full report at Codecov.
|
|
@jsuereth Thanks for the update, I rebuilt the container using this commit jsuereth/gogoproto@9c8da7e and regenerated the protos here |
The Sum field in the Histogram is now a *float64. Not sure if we need the floatPtr convenience method or if there's a better alternative.
|
@codeboten while discussing with @jsuereth we came with an alternative simpler solution, which is to use custom types for the "optional" fields that implement the "optional" part (since only double values). What do you think? |
|
@bogdandrutu you're thinking we could use the sed script to substitute the optional field with a custom type instead and use that to generated the protos or are you thinking of changing the field to be a custom type instead? |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@bogdandrutu / @jsuereth wondering if you could answer @codeboten's question. If I'm not mistaken the answer dictates whether we need to make changes to the protos or to the collector proto generation code. |
|
@jack-berg the conversation continued on this other PR (for the 2nd prototype). I'm now working through a third prototype to move away from gogo and use the standard protobuf library + the vtprotobuf extension |
|
Thanks for the update and for connecting the dots @codeboten! 👍 |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closing this for now. |
Description:
Draft PR only to capture the work needed to update gogoproto to support optional fields
Link to tracking Issue: Fixes #4258