`pdata`: Add support for optional double fields #4495

bogdandrutu · 2021-12-01T00:25:26Z

Description:
This is the second attempt at adding support for optional double fields in opentelemetry-proto. This implementation uses a customtype field in gogo protos.

Link to tracking Issue: #4258

Testing: Added unit tests

This is the second attempt at adding support for `optional double` fields in opentelemetry-proto. This implementation uses a customtype field in gogo protos.

bogdandrutu

The overall design is what I had in mind, only small details about how the "pdata" API exposes this.

model/internal/data/optional_double.go

proto_patch.sed

bogdandrutu · 2021-12-01T00:30:00Z

model/pdata/metrics.go

+func NewOptionalDouble(val float64) *data.OptionalDouble {
+	return data.NewOptionalDouble(val)
+}


"*data.OptionalDouble" is internal so you need to have something like "type OptionalDouble = data.OptionalDouble" and return that at least.

I think instead of changing HistogramDataPoint.Sum(OptionalDouble) we can have the same HistogramDataPoint.Sum() -> float64 and have a HistogramDataPoint.HasSum() -> bool?

The following changes were made: - make optional double field non-nullable - update pdata interface to use float64 instead of OptionalDouble type - added HasSum method to HistogramDataPoint - added serialization methods to OptionalDouble

codeboten · 2021-12-01T22:20:51Z

Updated the code based on the feedback. The following changes were made:

make optional double field non-nullable
update pdata interface to use float64 instead of OptionalDouble type
added HasSum method to HistogramDataPoint
added serialization methods to OptionalDouble

NOTE: If this looks like the way we want to go, I still need to update the template for generating the SetSum and Sum methods correctly for HistogramDataPoint.

codecov · 2021-12-01T22:27:33Z

Codecov Report

Merging #4495 (7165cb5) into main (49b1fd6) will decrease coverage by 0.03%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##             main    #4495      +/-   ##
==========================================
- Coverage   90.77%   90.74%   -0.04%     
==========================================
  Files         179      180       +1     
  Lines       10412    10443      +31     
==========================================
+ Hits         9452     9477      +25     
- Misses        743      749       +6     
  Partials      217      217

Impacted Files	Coverage Δ
model/pdata/metrics.go	`97.40% <0.00%> (-1.29%)`	⬇️
model/internal/data/optional_double.go	`86.20% <86.20%> (ø)`
model/internal/otlp_wrapper.go	`92.02% <100.00%> (ø)`
model/pdata/generated_metrics.go	`96.98% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49b1fd6...7165cb5. Read the comment docs.

bogdandrutu

I tried a lot to make this work with typecast as well as customtype, none of them really worked :( since the generated code either changes the field type (which will change the generated code) or does not know how to handle a pointer for a primitive float64:

See:

	if m.Sum != nil {
		if m.Sum != 0 {
			i -= 8
			encoding_binary.LittleEndian.PutUint64(dAtA[i:], uint64(math.Float64bits(float64(m.Sum))))
			i--
			dAtA[i] = 0x29
		}
	}

This should be:

	if m.Sum != nil {
		i -= 8
		encoding_binary.LittleEndian.PutUint64(dAtA[i:], uint64(math.Float64bits(float64(*m.Sum))))
		i--
		dAtA[i] = 0x29
	}

We have some options:

Change the generate code as well since there are 1-2 presence. We can do this to unblock the optional.
Move forward with @jsuereth prototype.
Try to use https://github.com/planetscale/vtprotobuf, it seems to be an active community and probably we can make further improvements there.
My long-term dream do a lazy custom parser. Probably lots of work for the moment.

bogdandrutu · 2021-12-02T18:02:00Z

internal/testdata/metric.go

@@ -234,14 +234,14 @@ func initHistogramMetric(hm pdata.Metric) {
 	hdp0.SetStartTimestamp(TestMetricStartTimestamp)
 	hdp0.SetTimestamp(TestMetricTimestamp)
 	hdp0.SetCount(1)
-	hdp0.SetSum(15)
+	hdp0.SetSum(float64(15))


Why this change?

bogdandrutu · 2021-12-02T18:03:34Z

model/internal/data/optional_double.go

+// OptionalDouble is a custom data type that is used for all optional double fields in OTLP
+// Protobuf messages.
+type OptionalDouble struct {
+	orig  *types.DoubleValue


Why pointer? Feels like unnecessary allocation.

bogdandrutu · 2021-12-02T18:32:51Z

model/internal/data/optional_double.go

+
+// MarshalTo converts OptionalDouble into a binary representation. Called by Protobuf serialization.
+func (m *OptionalDouble) MarshalTo(data []byte) (n int, err error) {
+	return m.orig.MarshalTo(data)


This is incorrect since the types.DoubleValue is a message with the float at ID=1 (see the https://github.com/gogo/protobuf/blob/v1.3.2/types/wrappers.pb.go#L1263). You need to only put the float value the ID will be filled by the parent.

codeboten · 2021-12-03T00:17:20Z

Change the generate code as well since there are 1-2 presence. We can do this to unblock the optional.

Move forward with @jsuereth prototype.

Try to use https://github.com/planetscale/vtprotobuf, it seems to be an active community and probably we can make further improvements there.

My long-term dream do a lazy custom parser. Probably lots of work for the moment.

@bogdandrutu @jsuereth

1 : so far this option seems pretty hacky, I could continue down this road but I'm not sure if I should.

2 & 3: if optional field support is just a one off that will allow us to move forward for some amount of time, then maybe it's not worth the effort to move off of gogo. On the other hand if it's going to be a repeated amount of work to continue adding features as protos evolve, maybe it's worth the investigation to see if the performance of vtproto would be acceptable for the collector.

4: my long term dream is not thinking about protobuf 🤣

MovieStoreGuy

I just had a comment on the library being used here.

MovieStoreGuy · 2021-12-03T05:15:57Z

model/internal/data/optional_double.go

+package data // import "go.opentelemetry.io/collector/model/internal/data"
+
+import (
+	"github.com/gogo/protobuf/types"


Hey @bogdandrutu ,

This project is currently unmaintained, is there any reason why not to use https://github.com/golang/protobuf ?

@MovieStoreGuy you can see more details on the discussion about what to do w/ gogo in the proto repo PR open-telemetry/opentelemetry-proto#336

Ultimately it comes down to performance, which is why we're investigating alternatives.

github-actions · 2021-12-11T03:15:47Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-12-31T03:16:03Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

bogdandrutu · 2022-01-04T22:07:03Z

@codeboten not sure if this is still needed, I think we discussed what we needed. Feel free to reopen.

CatherineF-dev · 2022-05-23T18:05:23Z

Thx, it's not related.

Hi Bogdan, do you think it's related to #5409 or not? Thanks!

pdata: Add support for optional double fields

6851ae6

This is the second attempt at adding support for `optional double` fields in opentelemetry-proto. This implementation uses a customtype field in gogo protos.

bogdandrutu requested review from a team and dmitryax December 1, 2021 00:25

bogdandrutu marked this pull request as draft December 1, 2021 00:25

bogdandrutu commented Dec 1, 2021

View reviewed changes

updates based on feedback

7165cb5

The following changes were made: - make optional double field non-nullable - update pdata interface to use float64 instead of OptionalDouble type - added HasSum method to HistogramDataPoint - added serialization methods to OptionalDouble

bogdandrutu commented Dec 2, 2021

View reviewed changes

MovieStoreGuy reviewed Dec 3, 2021

View reviewed changes

github-actions bot added the Stale label Dec 11, 2021

bogdandrutu removed the Stale label Dec 16, 2021

codeboten mentioned this pull request Dec 21, 2021

use gogo proto from jsuereth/protobuf fork for optional support #4259

Closed

github-actions bot added the Stale label Dec 31, 2021

bogdandrutu removed the Stale label Jan 3, 2022

bogdandrutu closed this Jan 4, 2022

codeboten mentioned this pull request Jan 12, 2022

Support optional fields in proto #4258

Closed

codeboten deleted the codeboten/support-optional-2 branch June 22, 2023 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`pdata`: Add support for optional double fields #4495

`pdata`: Add support for optional double fields #4495

bogdandrutu commented Dec 1, 2021 •

edited by codeboten

Loading

bogdandrutu left a comment

bogdandrutu Dec 1, 2021

codeboten commented Dec 1, 2021

codecov bot commented Dec 1, 2021

bogdandrutu left a comment

bogdandrutu Dec 2, 2021

bogdandrutu Dec 2, 2021

bogdandrutu Dec 2, 2021

codeboten commented Dec 3, 2021

MovieStoreGuy left a comment

MovieStoreGuy Dec 3, 2021

codeboten Dec 3, 2021 •

edited

Loading

github-actions bot commented Dec 11, 2021

github-actions bot commented Dec 31, 2021

bogdandrutu commented Jan 4, 2022

CatherineF-dev commented May 23, 2022 •

edited

Loading

pdata: Add support for optional double fields #4495

pdata: Add support for optional double fields #4495

Conversation

bogdandrutu commented Dec 1, 2021 • edited by codeboten Loading

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu Dec 1, 2021

Choose a reason for hiding this comment

codeboten commented Dec 1, 2021

codecov bot commented Dec 1, 2021

Codecov Report

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu Dec 2, 2021

Choose a reason for hiding this comment

bogdandrutu Dec 2, 2021

Choose a reason for hiding this comment

bogdandrutu Dec 2, 2021

Choose a reason for hiding this comment

codeboten commented Dec 3, 2021

MovieStoreGuy left a comment

Choose a reason for hiding this comment

MovieStoreGuy Dec 3, 2021

Choose a reason for hiding this comment

codeboten Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 11, 2021

github-actions bot commented Dec 31, 2021

bogdandrutu commented Jan 4, 2022

CatherineF-dev commented May 23, 2022 • edited Loading

`pdata`: Add support for optional double fields #4495

`pdata`: Add support for optional double fields #4495

bogdandrutu commented Dec 1, 2021 •

edited by codeboten

Loading

codeboten Dec 3, 2021 •

edited

Loading

CatherineF-dev commented May 23, 2022 •

edited

Loading