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

[pdata] Revisit ValueType.String() output #6249

Closed
dmitryax opened this issue Oct 6, 2022 · 5 comments
Closed

[pdata] Revisit ValueType.String() output #6249

dmitryax opened this issue Oct 6, 2022 · 5 comments
Labels
area:pdata pdata module related issues discussion-needed Community discussion needed

Comments

@dmitryax
Copy link
Member

dmitryax commented Oct 6, 2022

The output of the ValueType.String() is inconsistent with other similar methods. It has 2 issues:

  1. After applying [pdata] Revisit need of Val suffix of Value methods #6081, all the string value identifies were consistently renamed to Str. The short name Str is chosen instead of String in order to avoid unintentionally implementing fmt.Stringer interface by string type Value getter. Output of ValueType.String for string type is still STRING which is inconsistent with other types:
ValueTypeEmpty -> "EMPTY"
ValueTypeStr -> "STRING"
ValueTypeBool -> "BOOL"
ValueTypeInt -> "INT"
ValueTypeDouble -> "DOUBLE"
ValueTypeMap -> "MAP"
ValueTypeSlice -> "SLICE"
ValueTypeBytes -> "BYTES"
  1. String value returned by pcommon.ValueType.String has been generated by uppercasing type identifiers while other similar methods return identifies as e.g.:
MetricTypeNone -> "None"
MetricTypeGauge -> "Gauge"
MetricTypeSum -> "Sum"
MetricTypeHistogram -> "Histogram"
MetricTypeExponentialHistogram -> "ExponentialHistogram"
MetricTypeSummary -> "Summary"

Suggestion is to change ValueType.String() output to return the string representation of corresponding type identifiers:

ValueTypeEmpty -> "Empty"
ValueTypeStr -> "Str"
ValueTypeBool -> "Bool"
ValueTypeInt -> "Int"
ValueTypeDouble -> "Double"
ValueTypeMap -> "Map"
ValueTypeSlice -> "Slice"
ValueTypeBytes -> "Bytes"

It means we choose consistency over readability ("Str" instead of "String"). I believe it's ok given that we already return "BOOL" not "Boolean" for ValueTypeBool, but I'm open for any other suggestions.

cc @open-telemetry/collector-approvers

@dmitryax dmitryax added discussion-needed Community discussion needed area:pdata pdata module related issues labels Oct 6, 2022
@dmitryax dmitryax changed the title [pdata] Revisit ValueType.String() [pdata] Revisit ValueType.String() output Oct 6, 2022
@dmitryax dmitryax added discussion-needed Community discussion needed and removed discussion-needed Community discussion needed labels Oct 6, 2022
@bogdandrutu
Copy link
Member

bogdandrutu commented Oct 6, 2022

One more enum that we don't do this is plog.SeverityNumber see https:/open-telemetry/opentelemetry-collector/blob/main/pdata/plog/logs.go#L73

Update: an argument can be made that the enums defined in the proto have the String method as the proto string representation, but we don't do that for other places like StatusCode and MetricAggregationTemporality.

@dmitryax
Copy link
Member Author

dmitryax commented Oct 6, 2022

Makes sense. I'm looking into how the String() method is being used in contrib. I think we can make it shorter and update plog.SeverityNumber accordingly

@bogdandrutu
Copy link
Member

Please open a different issue about enums defined in the proto file. The enums Type defined by us are consistent now.

@bogdandrutu
Copy link
Member

Also make sure we look at all of them and we come up with the rules

@dmitryax
Copy link
Member Author

dmitryax commented Oct 7, 2022

Sounds good. Closing this issue in favor of #6251

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:pdata pdata module related issues discussion-needed Community discussion needed
Projects
None yet
Development

No branches or pull requests

2 participants