Update Metric Point for Counter and Gauge #2667

utpilla · 2021-11-23T22:44:34Z

Changes

Added public methods GetCounterSumLong, GetCounterSumDouble, GetGaugeLastValueLong, GetGaugeLastValueDouble, GetStartTime, and GetEndTime. to MetricPoint
Removed the public properties for these

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Changes in public API reviewed

codecov · 2021-11-23T22:55:22Z

Codecov Report

Merging #2667 (bdf52d3) into main (f7c718e) will decrease coverage by 0.04%.
The diff coverage is 89.36%.

@@            Coverage Diff             @@
##             main    #2667      +/-   ##
==========================================
- Coverage   82.92%   82.88%   -0.05%     
==========================================
  Files         249      249              
  Lines        8691     8705      +14     
==========================================
+ Hits         7207     7215       +8     
- Misses       1484     1490       +6

Impacted Files	Coverage Δ
src/OpenTelemetry/Metrics/MetricPoint.cs	`85.48% <80.00%> (-2.24%)`	⬇️
...tryProtocol/Implementation/MetricItemExtensions.cs	`90.64% <100.00%> (ø)`
...ometheus/Implementation/PrometheusSerializerExt.cs	`100.00% <100.00%> (ø)`
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`76.47% <0.00%> (-5.89%)`	⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	`97.14% <0.00%> (-2.86%)`	⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	`73.33% <0.00%> (-0.96%)`	⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs	`97.65% <0.00%> (+0.78%)`	⬆️
src/OpenTelemetry/Metrics/MetricTypeExtensions.cs	`20.00% <0.00%> (+20.00%)`	⬆️

cijothomas · 2021-11-23T23:43:51Z

src/OpenTelemetry/Metrics/MetricPoint.cs

+            }
+        }
+
+        public long GetLastLongValue()


Suggested change

public long GetLastLongValue()

public long GetLastValueLong()

Is "last value" something meaningful from the spec? If not something like GetGaugeValueLong might be more clear.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#last-value-aggregation

"Last Value" https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#last-value-aggregation

"Gauge" https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#gauge

Basically, Gauge in OpenTelemetry is a stream of metric points, and each point is a combination of {last value, timestamps, attributes, exemplars}.

We do have some helper methods with "Histogram" in the name (at least currently), what if we added a type in all the names? Something like...

GetCounterSumAsLong, GetCounterSumAsDouble, GetGaugeLastValueAsLong, GetGaugeLastValueAsDouble

^ I like this, except the "As" in the name which indicates we are doing some cast..
GetCounterSumLong and friends, sounds better to me.

cijothomas · 2021-11-23T23:44:17Z

src/OpenTelemetry/Metrics/MetricPoint.cs


-        internal MetricPointStatus MetricPointStatus { get; private set; }
+        public double GetDoubleSum()


Suggested change

public double GetDoubleSum()

public double GetSumDouble()

^ thoughts?

GetSum<double> 😆 (return value sucks)

To me GetDoubleSum reads better than GetSumDouble.
But: below I think it makes sense to call GetLastValueLong as others suggested. So.. now I'm in doubt on this one. It would be kinda inconsistent then if we go with GetDoubleSum, no? 🤔

@joaopgrassi #2667 (comment) we currently settled with this. Please let us know what do you think.

src/OpenTelemetry/CHANGELOG.md

joaopgrassi · 2021-11-24T08:32:51Z

src/OpenTelemetry/Metrics/MetricPoint.cs


-        internal MetricPointStatus MetricPointStatus { get; private set; }
+        public double GetDoubleSum()


To me GetDoubleSum reads better than GetSumDouble.
But: below I think it makes sense to call GetLastValueLong as others suggested. So.. now I'm in doubt on this one. It would be kinda inconsistent then if we go with GetDoubleSum, no? 🤔

src/OpenTelemetry/Metrics/MetricPoint.cs

joaopgrassi · 2021-11-24T08:54:17Z

src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs

+                            cursor = WriteLong(buffer, cursor, metricPoint.GetLongSum());
+                        }
+                        else
+                        {


It seems we do this code in a few places now (if sum - GetLongSum..). Maybe we could use an extension method on metricPoint that accepts the MetricType?

These checks be avoided. We have a TODO in line 68 about this.

…Point-For-Counter-Gauge

…//github.com/utpilla/opentelemetry-dotnet into utpilla/Update-MetricPoint-For-Counter-Gauge

Add public methods to get startTime, endTime, and metric values

f2bccb5

utpilla requested a review from a team November 23, 2021 22:44

Update CHANGELOG.md

22e7cef

Merge with origin/main

924b305

cijothomas reviewed Nov 23, 2021

View reviewed changes

Merge branch 'main' into utpilla/Update-MetricPoint-For-Counter-Gauge

bc0ae99

joaopgrassi reviewed Nov 24, 2021

View reviewed changes

cijothomas and others added 6 commits November 24, 2021 09:52

Merge branch 'main' into utpilla/Update-MetricPoint-For-Counter-Gauge

fa20922

Update CHANGELOG.md

825a789

Merge remote-tracking branch 'origin/main' into utpilla/Update-Metric…

206b99b

…Point-For-Counter-Gauge

Rename the public methods

ea9ccce

Merge branch 'utpilla/Update-MetricPoint-For-Counter-Gauge' of https:…

67897e4

…//github.com/utpilla/opentelemetry-dotnet into utpilla/Update-MetricPoint-For-Counter-Gauge

Fix markdown lint issues

1472510

cijothomas approved these changes Nov 24, 2021

View reviewed changes

Merge branch 'main' into utpilla/Update-MetricPoint-For-Counter-Gauge

bdf52d3

cijothomas merged commit 9e83587 into open-telemetry:main Nov 24, 2021

utpilla mentioned this pull request Nov 29, 2021

MetricPoint start and end time as Properties #2703

Merged

utpilla deleted the utpilla/Update-MetricPoint-For-Counter-Gauge branch November 3, 2023 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Metric Point for Counter and Gauge #2667

Update Metric Point for Counter and Gauge #2667

utpilla commented Nov 23, 2021 •

edited

Loading

codecov bot commented Nov 23, 2021 •

edited

Loading

cijothomas Nov 23, 2021

CodeBlanch Nov 23, 2021 •

edited

Loading

cijothomas Nov 23, 2021

reyang Nov 23, 2021

reyang Nov 24, 2021

CodeBlanch Nov 24, 2021

cijothomas Nov 24, 2021

cijothomas Nov 23, 2021

reyang Nov 24, 2021 •

edited

Loading

joaopgrassi Nov 24, 2021

cijothomas Nov 24, 2021

joaopgrassi Nov 24, 2021

joaopgrassi Nov 24, 2021

cijothomas Nov 24, 2021

	public long GetLastLongValue()
	public long GetLastValueLong()


		internal MetricPointStatus MetricPointStatus { get; private set; }
		public double GetDoubleSum()

Update Metric Point for Counter and Gauge #2667

Update Metric Point for Counter and Gauge #2667

Conversation

utpilla commented Nov 23, 2021 • edited Loading

Changes

codecov bot commented Nov 23, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

CodeBlanch Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla commented Nov 23, 2021 •

edited

Loading

codecov bot commented Nov 23, 2021 •

edited

Loading

CodeBlanch Nov 23, 2021 •

edited

Loading

reyang Nov 24, 2021 •

edited

Loading