-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: add support for OT ValueRecorder #130
feat: add support for OT ValueRecorder #130
Conversation
Codecov Report
@@ Coverage Diff @@
## master #130 +/- ##
==========================================
+ Coverage 95.29% 95.36% +0.06%
==========================================
Files 11 11
Lines 340 345 +5
Branches 68 70 +2
==========================================
+ Hits 324 329 +5
Misses 16 16
Continue to review full report at Codecov.
|
function transformValueType(valueType: OTValueType): ValueType { | ||
if (valueType === OTValueType.DOUBLE) { | ||
function transformValueType(valueType: OTValueType, point: OTPoint): ValueType { | ||
if (isDistributionValue(point.value) || isHistogramValue(point.value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the JS SDK, but looking through this code, isDistribution()
will be true for ValueRecorder
. Is that right?
I'm confused because transformValue()
will then throw on L242.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's correct. I've opened a separate PR to address the support for Distribution
and Histogram
type metrics to be exported here: #125
When testing with those changes, I've gotten the ValueRecorder
to correctly export.
Who is responsible of merging that? |
This PR needs to be updated and it should take a minor change to send LastValue |
The OTel metrics specification still hasn't decided what default aggregator will be used for ValueRecorder. It's looking like it will be "some histogram sketch" (e.g. DDSketch) which will produce fancy histograms. Since this will be changing, closing this PR. |
Fixes: #129
Changes
ValueRecorder
distribution metricsValueRecorder
manually on a personal GCP projectFeedback Request
transformValueType
function needs themetric.aggregator.toPoint().value
to disambiguate betweenDistribution
/Histogram
data types. Please let me know if you think this should be passed down totransformValueType
in a different manner than shown.