-
Notifications
You must be signed in to change notification settings - Fork 824
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: updates ValueRecorder to allow negative values #1373
fix: updates ValueRecorder to allow negative values #1373
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1373 +/- ##
==========================================
+ Coverage 93.99% 94.09% +0.09%
==========================================
Files 153 153
Lines 4666 4656 -10
Branches 963 959 -4
==========================================
- Hits 4386 4381 -5
+ Misses 280 275 -5
|
correlationContext?: api.CorrelationContext, | ||
spanContext?: api.SpanContext | ||
): void { | ||
if (this._absolute && value < 0) { |
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.
This is the critical crux of the change.
record(value: number, correlationContext: CorrelationContext): void; | ||
record( | ||
value: number, | ||
correlationContext: CorrelationContext, | ||
spanContext: SpanContext | ||
): void; |
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.
These signatures appeared completely unused. When get to the BoundValueRecorder
they were just ignored, so I removed them. Have an open question in Gitter but figured I'd start with removing and can always add back if necessary.
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 believe these signatures came from the spec, but the spec on what to do with the correlations/span context was never finished.
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.
Ah, interesting. Feels like these should be left out until that is known but obviously I am new to the project and may not understand all the impacts.
I couldn't find any current usages. May be some usages in the wild outside of these repos (js/contrib) but we are also < 1.0 so figured was OK to have a breaking change to clean-up at this point.
* (Measure only, default true) Asserts that this metric will only accept | ||
* non-negative values (e.g. disk usage). | ||
*/ | ||
absolute?: boolean; |
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.
ValueRecorder was the only thing using absolute
that I could find. Via #1241 it seems like that should not be configurable. As such, seemed natural to rip out while in here.
const boundValueRecorder = valueRecorder.bind(labels); | ||
boundValueRecorder.record(-10); | ||
boundValueRecorder.record(10); |
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.
This diff makes things confusing but that test case was not touched, its just a shifting of code.
}); | ||
|
||
it( | ||
'should accept negative (and positive) values when absolute is set' + |
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.
This test case was modified to not pass any value for 'absolute' and then use existing assertions to assert both positive and negative values are accepted.
I think codecov is mad at me for deleting too much covered code? 🤷 |
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.
LGTM
Looks like the context parameters to the record method weren't being tested, so I think it's fine to remove them. They can always be readded when the associated functionality is built out.
Haha actually it's mad because only 75% of lines you added are tested. Since you mostly deleted and only added a very small number of lines, it is saying you only tested 75% of the diff, even though you increased total project coverage by 0.07%. |
I believe the only actual new non-test code I added was appropriately referencing the Also, the related issue is flagged as required for GA so not sure if you want to flag this PR or not for the project board but figured I'd mention. |
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.
lgtm, but indeed the way it is shown in git it suggests you have swapped 2 tests :)
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.
LGTM
@dyladan merged in master. think this has the necessary approvals now (2 maintainers, 3 total reviewers) |
Which problem is this PR solving?
Short description of the changes
absolute
configuration since no-longer has a valid userecord(..., correlationContext, spanContext)
signature(s)