-
Notifications
You must be signed in to change notification settings - Fork 858
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
Add toString to SdkMeter, SdkObservableInstrument, AbstractInstrumentBuilder #5072
Conversation
@@ -33,9 +35,13 @@ | |||
AbstractInstrumentBuilder( |
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 refactoring in this class ensures all descriptive fields required for a toString()
implementation are available when constructed.
@@ -28,18 +27,20 @@ public final class CallbackRegistration { | |||
private final ThrottlingLogger throttlingLogger = new ThrottlingLogger(logger); | |||
private final List<SdkObservableMeasurement> observableMeasurements; | |||
private final Runnable callback; | |||
private final String callbackDescription; |
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 callbackDescription
field was really a toString()
implementation in disguise.
|
||
Builder( | ||
SdkDoubleCounterBuilder( |
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.
Giving these builders more descriptive names gives AbstractInstrumentBuilder#toString()
a useful classname to include.
Codecov ReportBase: 91.07% // Head: 91.09% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #5072 +/- ##
============================================
+ Coverage 91.07% 91.09% +0.02%
- Complexity 4889 4891 +2
============================================
Files 553 553
Lines 14469 14453 -16
Branches 1381 1381
============================================
- Hits 13177 13166 -11
+ Misses 898 894 -4
+ Partials 394 393 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
WriteableMetricStorage storage = | ||
meterSharedState.registerSynchronousMetricStorage(descriptor, meterProviderSharedState); | ||
return instrumentFactory.apply(descriptor, storage); | ||
} | ||
|
||
final CallbackRegistration registerDoubleAsynchronousInstrument( | ||
final SdkObservableInstrument registerDoubleAsynchronousInstrument( | ||
InstrumentType type, Consumer<ObservableDoubleMeasurement> updater) { |
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.
isn't the type available now at the class level, and hence not needed to be passed in?
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.
oh, I think I see... for types that can be both observable and not, we need to be able to pass this in, correct?
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.
Yes exactly. The type is assumed to be the synchronous version (i.e. COUNTER, UP_DOWN_COUNTER, etc) until a callback is regsitered.
Resolves #5013.
The sample code I posted here now prints: