[Exporter.Geneva] Update GenevaMetricExporter to export Exemplars #1069
Conversation
github-actions
bot
assigned cijothomas, CodeBlanch, mic-max, reyang, utpilla and Yun-Ting
github-actions
bot
requested review from
cijothomas,
CodeBlanch,
mic-max,
reyang and
Yun-Ting
utpilla
added
the
comp:exporter.geneva
…MetricExporter-to-use-TLV
| } | ||
|
|
||
| var bodyLength = this.SerializeHistogramMetric( | ||
| _ = metricPoint.TryGetHistogramMinMaxValues(out double min, out double max); |
There was a problem hiding this comment.
if min max is not available, dont we need ulongZero to be written?
There was a problem hiding this comment.
Yes, it would serialize them as zero. If TryGetHistogramMinMaxValues returns false, min and max would be set to zero.
There was a problem hiding this comment.
mm.. Is that a contract or relying on the internal implementation details on OTel sdk? I think its wrong to rely on the min, max if TryGet fails.
There was a problem hiding this comment.
That should be considered a breaking change from the SDK standpoint: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Metrics/MetricPoint.cs#L328-L330
There was a problem hiding this comment.
I am not sure if that is the case. Once it returned false, callers should not be relying on the values on the out parameters.
There was a problem hiding this comment.
I can explicitly set them to zero if the method returns false, but I do think the SDK should not change the behavior of this public API when it returns false. Also, it doesn't make sense for the SDK to assign to them any value other than the default value of zero when it returns false.
| payloadTypeStartIndex = bufferIndex; | ||
| bufferIndex += 2; | ||
|
|
||
| MetricSerializer.SerializeByte(this.buffer, ref bufferIndex, 0); // version |
There was a problem hiding this comment.
nit: consider moving these to a const elsewhere.
| MetricSerializer.SerializeByte(this.buffer, ref bufferIndex, 0); // version | ||
|
|
||
| var exemplarsCount = 0; | ||
| foreach (var exemplar in exemplars) |
There was a problem hiding this comment.
lets add a TODO to avoid this dual iteration, and instead backfill the count
There was a problem hiding this comment.
I don't think we can avoid it. The count field doesn't have a predefined size. It could be anywhere from 1 to 9 bytes. So we need to know the count beforehand.
| bool hasLabels = exemplar.FilteredTags != null && exemplar.FilteredTags.Count > 0; | ||
| byte numberOfLabels = 0; | ||
|
|
||
| var unixNanoSeconds = DateTime.FromFileTimeUtc(exemplar.Timestamp.ToFileTime()) |
There was a problem hiding this comment.
good to extract to a helper method.
There was a problem hiding this comment.
This is the only method that needs it 😄. This helper method could be a comment.
src/OpenTelemetry.Exporter.Geneva/Metrics/GenevaMetricExporter.cs
Outdated
Show resolved
Hide resolved
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1069 +/- ##
=======================================
Coverage ? 68.54%
=======================================
Files ? 210
Lines ? 8113
Branches ? 0
=======================================
Hits ? 5561
Misses ? 2552
Partials ? 0
|
Changes
1.5.0-alpha.1version of OpenTelemetry SDKTODO:
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes