-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
use sed to translate optional -> oneof #4929
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4929 +/- ##
==========================================
+ Coverage 90.85% 91.04% +0.18%
==========================================
Files 180 178 -2
Lines 10622 10664 +42
==========================================
+ Hits 9651 9709 +58
+ Misses 755 738 -17
- Partials 216 217 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. I think this is a good compromise to move things forward with the optional, and unblock proto changes
This supports adding min/max optional fields.
Output from sending min/max for histo/expohisto datapoints:
Will leave this in draft until open-telemetry/opentelemetry-proto#279 is merged |
@tigrannajaryan please take a look at this draft as you've reviewed the PR this replaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
// SetMin replaces the min associated with this HistogramDataPoint. | ||
func (ms HistogramDataPoint) SetMin(v float64) { | ||
(*ms.orig).Min_ = &otlpmetrics.HistogramDataPoint_Min{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this can check to see if (*ms.orig).Min_ != nil
and use it without allocating a new one.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closing this, changes will be incorporated into #5064 |
Using sed to substitute
optional
fields withoneof
as suggested by @bogdandrutu: #4258 (comment)This is a better alternative than #4750 as it allows us to continue using the proto generation as before.
Fixes #4258