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

Refactor for DataValue #2093

Merged
merged 2 commits into from
Jul 6, 2021
Merged

Refactor for DataValue #2093

merged 2 commits into from
Jul 6, 2021

Conversation

victlu
Copy link
Contributor

@victlu victlu commented Jun 18, 2021

Refactor out value portion of IDataPoint to a IDataValue.

@victlu victlu requested review from cijothomas and a team June 18, 2021 23:10
@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #2093 (060d29d) into metrics (ed3322c) will decrease coverage by 0.04%.
The diff coverage is 30.64%.

Impacted file tree graph

@@             Coverage Diff             @@
##           metrics    #2093      +/-   ##
===========================================
- Coverage    83.00%   82.96%   -0.05%     
===========================================
  Files          209      211       +2     
  Lines         6698     6694       -4     
===========================================
- Hits          5560     5554       -6     
- Misses        1138     1140       +2     
Impacted Files Coverage Δ
src/OpenTelemetry/Metrics/DataPoint/DataPoint.cs 0.00% <0.00%> (-37.50%) ⬇️
...rc/OpenTelemetry/Metrics/DataPoint/DataPoint{T}.cs 0.00% <0.00%> (ø)
src/OpenTelemetry/Metrics/DataPoint/Exemplar.cs 0.00% <0.00%> (ø)
src/OpenTelemetry/Metrics/DataPoint/Exemplar{T}.cs 0.00% <0.00%> (ø)
src/OpenTelemetry/Metrics/DataPoint/DataValue.cs 55.55% <55.55%> (ø)
...y/Metrics/MetricAggregators/SumMetricAggregator.cs 80.51% <71.42%> (+1.85%) ⬆️
...rc/OpenTelemetry/Metrics/DataPoint/DataValue{T}.cs 100.00% <100.00%> (ø)
...Metrics/MetricAggregators/GaugeMetricAggregator.cs 100.00% <100.00%> (+27.50%) ⬆️
...emetry/Metrics/Processors/MetricConsoleExporter.cs 88.00% <100.00%> (-4.00%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
... and 6 more

Copy link
Member

@pirgeo pirgeo left a comment

Choose a reason for hiding this comment

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

LGTM, I just have one question: In the proto and the spec, Attribute or Label is used as the name for Key-Value pairs, whereas here, Tag is used - I might be comparing apples to oranges here, so if I do, please let me know, but would it make sense to keep the naming consistent everywhere as much as possible? Or is there some guidance on when to use what that that I simply missed?

@victlu
Copy link
Contributor Author

victlu commented Jun 28, 2021

@pirgeo You are correct. We need to rename tag to attribute. Just haven't gotten to it yet.

@cijothomas cijothomas merged commit 95a1367 into open-telemetry:metrics Jul 6, 2021
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.

4 participants