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.

Sign up for GitHub

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

Jump to bottom

[pdata] Split pmetric.MetricValueType into more specific types #5233

Merged
merged 2 commits into from
Apr 20, 2022
Merged

[pdata] Split pmetric.MetricValueType into more specific types #5233

merged 2 commits into from
Apr 20, 2022
+107 −48

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Apr 19, 2022 •
edited
Loading

Split pmetric.MetricValueType into NumberDataPointValueType and ExemplarValueType

  1. pmetric.NumberDataPoint.ValueType() now returns a NumberDataPointValueType that can be NumberDataPointValueTypeInt or NumberDataPointValueTypeDouble.

  2. pmetric.Exemplar.ValueType() now returns a ExemplarValueType that can be ExemplarValueInt or ExemplarValueDouble.

(1) use case is most commonly being used. Thats why we keep backward compatibility for it. We don't keep backward compatibility for (2) it to avoid mixing the split types.

Closes: #4819

Replaces #5227

@dmitryax dmitryax requested review from a team and bogdandrutu April 19, 2022 21:03
@codecov
Copy link

codecov bot commented Apr 19, 2022 •
edited
Loading

Codecov Report

Merging #5233 (43affeb) into main (87069a8) will increase coverage by 0.00%.
The diff coverage is 92.85%.

@@           Coverage Diff           @@
##             main    #5233   +/-   ##
=======================================
  Coverage   90.59%   90.60%           
=======================================
  Files         187      187           
  Lines       11052    11061    +9     
=======================================
+ Hits        10013    10022    +9     
  Misses        820      820           
  Partials      219      219
Impacted Files Coverage Δ
pdata/internal/generated_pmetric.go 97.21% <83.33%> (ø)
internal/otlptext/databuffer.go 100.00% <100.00%> (ø)
pdata/internal/metrics.go 92.50% <100.00%> (+0.60%) ⬆️

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 87069a8...43affeb. Read the comment docs.

@dmitryax dmitryax force-pushed the split-value-type branch 2 times, most recently from e32ec39 to 66f545a Compare April 19, 2022 21:10
@dmitryax dmitryax mentioned this pull request Apr 19, 2022
Closed
bogdandrutu
bogdandrutu reviewed Apr 19, 2022
View reviewed changes
pdata/pmetric/alias.go Outdated
Comment on lines 108 to 107
// ExemplarValueType specifies the type of Exemplar measurement value.
type ExemplarValueType = internal.MetricValueType

const (
ExemplarValueTypeNone = internal.MetricValueTypeNone
ExemplarValueTypeInt = internal.MetricValueTypeInt
ExemplarValueTypeDouble = internal.MetricValueTypeDouble
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is that used, so I would propose an exception here and do the correct thing, as long as we can build successfully the contrib. Give a try :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea is to see if we do the right thing for exemplar (no backwards compatible change) is ok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. We can probably break MetricValueType -> ExemplarValueType, but keep MetricValueType <-> NumberDataPOintValueType backward compatible 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR. Looks like Exemplar types are used at in pkg/translator/opencensus/metrics_to_oc.go I don't see any other errors

@dmitryax
[pdata] Split pmetric.MetricValueType into specific types

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
31b6e75 
Split `pmetric.MetricValueType` into `NumberDataPointValueType` and `ExemplarValueType`

1. `NumberDataPoint.ValueType()` now returns a `NumberDataPointValueType` that can be `NumberDataPointValueTypeInt` or `NumberDataPointValueTypeDouble`.

2. `pmetric.Exemplar.ValueType()` now returns a `ExemplarValueType` that can be `ExemplarValueInt` or `ExemplarValueDouble`.

1 Use case is the most common. Thats why we are keeping backward compatibility for it. 2 is rarely being used. So we don't keep backward compatibility for it to avoid mixing the split types.
bogdandrutu
bogdandrutu approved these changes Apr 19, 2022
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with the exception for the ExemplarValueType.

cc @codeboten @tigrannajaryan

@tigrannajaryan
Copy link
Member

I am ok with the exception for the ExemplarValueType.

cc @codeboten @tigrannajaryan

SGMT, please check the contrib codebase for usage too and if it is very rarely used then I am fine with this approach.

@bogdandrutu bogdandrutu merged commit 24cf5c4 into open-telemetry:main Apr 20, 2022
@dmitryax dmitryax deleted the split-value-type branch April 21, 2022 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten

@bogdandrutu bogdandrutu

@tigrannajaryan tigrannajaryan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[pdata] Consider renaming of MetricValueType
4 participants
@dmitryax @tigrannajaryan @codeboten @bogdandrutu