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

use sed to translate optional -> oneof #4929

Closed
wants to merge 4 commits into from

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Feb 28, 2022

Using sed to substitute optional fields with oneof as suggested by @bogdandrutu: #4258 (comment)

@codeboten one option which I don't know if we thought/discuss about is to actually rely on oneof for the "optional/pointer":
optional fixed64 foo = 1 -> oneof foo_ { fixed64 foo = 1;}

This is a better alternative than #4750 as it allows us to continue using the proto generation as before.

Fixes #4258

@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #4929 (b297782) into main (67527b4) will increase coverage by 0.18%.
The diff coverage is 93.65%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
model/pdata/metrics.go 94.23% <42.85%> (-3.71%) ⬇️
model/pdata/generated_metrics.go 97.29% <100.00%> (+0.15%) ⬆️
config/configtelemetry/configtelemetry.go 90.32% <0.00%> (ø)
config/configmapprovider/retrieved.go
config/configmapprovider/provider.go
service/telemetry.go 76.25% <0.00%> (+14.89%) ⬆️

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 67527b4...b297782. Read the comment docs.

Copy link
Member

@bogdandrutu bogdandrutu left a 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

Alex Boten added 3 commits March 1, 2022 15:21
@codeboten
Copy link
Contributor Author

Output from sending min/max for histo/expohisto datapoints:

    metrics {
      name: "histogram"
      unit: "1"
      histogram {
        data_points {
          sum: 1234.0
          min: 10.0
          max: 694.1
        }
        aggregation_temporality: AGGREGATION_TEMPORALITY_CUMULATIVE
      }
    }
    metrics {
      name: "exponential-histogram"
      unit: "1"
      exponential_histogram {
        data_points {
          sum: 12345.0
          positive {
          }
          negative {
          }
          min: 105.0
          max: 694.15
        }
        aggregation_temporality: AGGREGATION_TEMPORALITY_DELTA
      }
    }

Will leave this in draft until open-telemetry/opentelemetry-proto#279 is merged

@codeboten
Copy link
Contributor Author

@tigrannajaryan please take a look at this draft as you've reviewed the PR this replaces.

Copy link
Member

@tigrannajaryan tigrannajaryan left a 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{
Copy link
Member

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.

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Mar 17, 2022
@codeboten codeboten removed the Stale label Mar 17, 2022
@codeboten
Copy link
Contributor Author

Closing this, changes will be incorporated into #5064

@codeboten codeboten closed this Mar 24, 2022
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.

Support optional fields in proto
3 participants