add optional fields manually #4750

codeboten · 2022-01-25T20:22:34Z

Description:
The following PR manually adds support for Min/Max optional fields to ExponentialHistogramDataPoint/HistogramDataPoint. Some things to note:

Once this PR is merged, we will no longer be able to use genproto to update the proto files as this is editing them manually.
Ideally in the future we would move all the fields to serialize/deserialize directly from pdata, that being said I didn't want to start with such a major refactor here.
Cleanup to remove proto generation code could also be done here, if we want to move forward with this change, I will open a separate PR to address this.

I believe once there is a pattern for a given field type, this approach should be easy enough to follow.

Link to tracking Issue: #4258

Testing: ~~Added tests for serializing/deserializing the data. I plan on doing some manual tests with a mock server that uses the proto as generated by the google proto library.~~ This has been done: #4750 (comment)

bogdandrutu · 2022-02-02T02:22:07Z

model/pdata/optional.go

+// IsEmpty returns true if id doesn't contain at least one non-zero byte.
+func (t OptionalDouble) IsEmpty() bool {
+	return t.orig.IsEmpty()
+}


Would be good to be kind of in sync with generated protobuf code, is this how they call the func?

The generated protos checks for nil values. You can see the generated protos here

codeboten@7811abd#diff-7276c207e936f474133fad60cb485d044f4a8cc3931300c5b82b34fa2a238abdR810-R821

Here's an example of GetMin:

func (x *HistogramDataPoint) GetMin() float64 { if x != nil && x.Min != nil { return *x.Min } return 0 }

It uses the following to marshal:

if m.Max != nil { i -= 8 binary.LittleEndian.PutUint64(dAtA[i:], uint64(math.Float64bits(float64(*m.Max)))) i-- dAtA[i] = 0x61 } if m.Min != nil { i -= 8 binary.LittleEndian.PutUint64(dAtA[i:], uint64(math.Float64bits(float64(*m.Min)))) i-- dAtA[i] = 0x59 }

Following the IsEmpty pattern was done to remain consistent with the trace/span ID custom types already present in pdata.

bogdandrutu · 2022-02-02T02:28:19Z

model/pdata/generated_metrics.go

+// Min returns the min associated with this HistogramDataPoint.
+func (ms HistogramDataPoint) Min() OptionalDouble {
+	return OptionalDouble{orig: ((*ms.orig).Min)}
+}
+
+// SetMin replaces the min associated with this HistogramDataPoint.
+func (ms HistogramDataPoint) SetMin(v OptionalDouble) {
+	(*ms.orig).Min = v.orig
+}


Another approach:

// Min returns the min associated with this HistogramDataPoint. func (ms HistogramDataPoint) Min() float64 { return (*ms.orig).Min.DoubleValue } func (ms HistogramDataPoint) HasMin() bool { !(*ms.orig).Min.IsEmpty() } func (ms HistogramDataPoint) SetMin(v float64) { (*ms.orig).Min.DoubleValue = v }

Are you suggesting we do away with the OptionalDouble type in pdata? I don't have a strong preference to keeping it and this approach would give us essentially the same functionality without an additional type.

That is my idea.. What do you think?

Just tried it and I agree it's better. I've added the HasMin HasMax methods to metrics.go instead of creating a new optional field generator. We can always add one later if needed.

Decided to add a template for optional fields after all, since this will be re-used for min/max and sum in the near future. PTAL @bogdandrutu

codecov · 2022-02-02T18:08:05Z

Codecov Report

Merging #4750 (620032e) into main (a7ef079) will decrease coverage by 0.01%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main    #4750      +/-   ##
==========================================
- Coverage   90.70%   90.69%   -0.02%     
==========================================
  Files         179      180       +1     
  Lines       10566    10636      +70     
==========================================
+ Hits         9584     9646      +62     
- Misses        763      771       +8     
  Partials      219      219

Impacted Files	Coverage Δ
model/pdata/generated_metrics.go	`96.33% <71.42%> (-0.74%)`	⬇️
internal/otlptext/databuffer.go	`100.00% <100.00%> (ø)`
model/internal/data/optional.go	`100.00% <100.00%> (ø)`
component/componenttest/nop_exporter.go	`100.00% <0.00%> (ø)`
component/componenttest/nop_receiver.go	`100.00% <0.00%> (ø)`
component/componenttest/nop_extension.go	`100.00% <0.00%> (ø)`
component/componenttest/nop_processor.go	`100.00% <0.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 a7ef079...620032e. Read the comment docs.

codeboten · 2022-02-04T17:47:16Z

To validate the proto works as expected, I tested this PR against a Python gRPC server which used the generated code from open-telemetry/opentelemetry-proto#279. Here's the export request with min/max fields:

      histogram {
        data_points {
          count: 10
          bucket_counts: 21
          bucket_counts: 32
          min: 0.1
          max: 100.0
        }
      }

And here's another with only one of the fields:

      histogram {
        data_points {
          count: 10
          bucket_counts: 21
          bucket_counts: 32
          max: 100.0
        }
      }

codeboten · 2022-02-04T17:51:55Z

I believe to move this PR forward, I can split it into two PRs:

all the internals to support optionals (this is everything in pdata except adding the min/max fields). This could be merged before the protos to make adding the fields easy once the proto PRs merge.
the code that adds min/max + otlp serialization. This PR could either exist in draft mode or I can wait to open until the proto is merged.

Thoughts?

bogdandrutu · 2022-02-05T01:32:10Z

model/internal/data/optional.go

+	"math"
+)
+
+type OptionalDouble struct {


This is "OptionalFixed64" correct (is not the int64 from proto is the fixed64)?

Correct, this is a fixed64 field.

bogdandrutu · 2022-02-05T01:33:21Z

model/internal/data/protogen/metrics/v1/metrics.pb.go

+	if !m.Max.IsEmpty() {
+		i -= m.Max.Size()
+		if _, err := m.Max.MarshalTo(dAtA[i:]); err != nil {
+			return 0, err
+		}
+		dAtA[i] = 0x61
+	}
+	if !m.Min.IsEmpty() {
+		i -= m.Min.Size()
+		if _, err := m.Min.MarshalTo(dAtA[i:]); err != nil {
+			return 0, err
+		}
+		dAtA[i] = 0x59
+	}


Is this manually written? How can we maintain this?

Correct, the process here would be that it would have to be added back to the proto after make genproto is run. Any thoughts on how to best address this? I suppose we could add this marshal/unmarshal methods outside of the generated code, and remove the function as part of genproto?

I'm open to alternatives here.

Can we automate the patching as part of make genproto step? Also extract the manually written as much as possible as a func and move to a regular (non-generated) file and call it from here (minimize the line count of manual code present in this auto-generated file).

Also add a comment clearly saying where does the manual code in this file start and where it ends. This should help to locate the right place to manually patch after re-generation and if auto-patching fails.

github-actions · 2022-02-24T03:15:39Z

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

codeboten · 2022-03-02T18:07:40Z

Closing in favour of #4929

codeboten changed the title ~~Codeboten/add optional fields manually~~ add optional fields manually Jan 25, 2022

bogdandrutu reviewed Feb 2, 2022

View reviewed changes

codeboten force-pushed the codeboten/add-optional-fields-manually branch from cab5b78 to 68ed192 Compare February 2, 2022 18:04

Alex Boten added 9 commits February 3, 2022 10:12

add new OptionDouble type

e1cc7bd

add min/max to proto

414b8b0

add unmarshal, update serialization

55870d1

add min/max optional fields to exponential histogram datapoint

7423148

fix lint

ef468f6

add test for deserialization

ca1de09

remove OptionalDouble pdata type

5adcdad

fix lint

92b7423

add templating for optional values

620032e

codeboten force-pushed the codeboten/add-optional-fields-manually branch from dbf8f28 to 620032e Compare February 4, 2022 17:42

codeboten mentioned this pull request Feb 4, 2022

Allow field-presence to be used in OTLP open-telemetry/opentelemetry-proto#336

Closed

bogdandrutu reviewed Feb 5, 2022

View reviewed changes

github-actions bot added the Stale label Feb 24, 2022

codeboten closed this Mar 2, 2022

codeboten mentioned this pull request Mar 2, 2022

use sed to translate optional -> oneof #4929

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add optional fields manually #4750

add optional fields manually #4750

codeboten commented Jan 25, 2022 •

edited

Loading

bogdandrutu Feb 2, 2022

codeboten Feb 2, 2022

bogdandrutu Feb 2, 2022

codeboten Feb 2, 2022

bogdandrutu Feb 2, 2022

codeboten Feb 3, 2022

codeboten Feb 4, 2022

codecov bot commented Feb 2, 2022 •

edited

Loading

codeboten commented Feb 4, 2022

codeboten commented Feb 4, 2022

bogdandrutu Feb 5, 2022

codeboten Feb 7, 2022

bogdandrutu Feb 5, 2022

codeboten Feb 7, 2022

tigrannajaryan Feb 9, 2022

tigrannajaryan Feb 9, 2022

github-actions bot commented Feb 24, 2022

codeboten commented Mar 2, 2022

add optional fields manually #4750

add optional fields manually #4750

Conversation

codeboten commented Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 2, 2022 • edited Loading

Codecov Report

codeboten commented Feb 4, 2022

codeboten commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2022

codeboten commented Mar 2, 2022

codeboten commented Jan 25, 2022 •

edited

Loading

codecov bot commented Feb 2, 2022 •

edited

Loading