-
Notifications
You must be signed in to change notification settings - Fork 821
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
fix: ignore non-number value on BaseBoundInstrument.update #1366
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1366 +/- ##
==========================================
- Coverage 93.89% 93.80% -0.10%
==========================================
Files 148 144 -4
Lines 4393 4276 -117
Branches 894 869 -25
==========================================
- Hits 4125 4011 -114
+ Misses 268 265 -3 |
@@ -374,6 +375,63 @@ describe('Meter', () => { | |||
assert.strictEqual(record1.aggregator.toPoint().value, 20); | |||
assert.strictEqual(boundCounter, boundCounter1); | |||
}); | |||
|
|||
it('should trunk non-integer values for INT valueType', async () => { |
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.
what do you mean by "trunk" here?
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.
Most likely trunc or truncate. @legendecas Please avoid abbreviations.
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.
ignore
then ?
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.
No, it's a typo and it should be fixed, and the other cases should be made into their own tests.
it('should trunk non-integer values for INT valueType', async () => { | |
it('should truncate floating number values for INT valueType', async () => { |
valueType: ValueType.INT, | ||
}); | ||
const boundCounter = upDownCounter.bind(labels); | ||
{ |
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.
why extra brackets are needed here and in few other places ? If you want to separate cases perhaps it's a moment where it should go into separate unit test or simply make an array of values to test and then use a loop to test each 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.
The brackets give lexical scope to the const [record1] = meter.getBatcher().checkPointSet();
line. It is repeated several times in the test so that different values may be tested.
These should be separate tests as they are not all testing truncation
@obecny @vmarchaud looks like your concerns have been addressed? |
…utes (open-telemetry#1366) Co-authored-by: Haddas Bronfman <[email protected]>
Which problem is this PR solving?
counter.add(undefined)
orcounter.add({ arbitrary: 'value' })
. Hence those aggregators conforming to the type interface Aggregator may receive non-number value onAggregator.update
.Short description of the changes
number
onBaseBoundInstrument.update
.