Metric Observer instrument specification (refinement) #72
Conversation
jmacd
requested review from
AloisReitbauer,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
reyang,
SergeyKanzhelev,
songy23,
tedsuo,
tigrannajaryan and
yurishkuro
as code owners
There was a problem hiding this comment.
It seems that registering an observer does not require specifying any timing parameters, meaning that the timing of the callback invocations is fully controlled by the SDK. If that's the case, why is it necessary to introduce the new instrument type instead of just providing an ability to register anonymous callbacks invoked by the SDK according to its polling interval? Please add more color in the Motivation section.
|
@yurishkuro I've added two paragraphs to try to address your question. I will confess to having mixed feelings about this API. I volunteered to write this specification after I removed it from the metrics spec earlier, in an agreement with @bogdandrutu. My original draft in OTEP 0008 didn't talk much about about calling conventions, but I had not at that time been thinking of supporting "bound" observer instruments. The addition of bound observer instruments raises some questions (e.g., what prevents you from calling them in a different observer's callback?). Another possibility (@bogdandrutu please comment) would be to support a callback for unbound observers and individual callbacks for each bound observer. Why support one callback per instrument, not one callback per bound instrument IOW? |
|
(I've asked @bogdandrutu this in the past, and he expressed a preference for a single-callback as proposed here, not a callback per bound observer. It's fewer allocations.) |
|
@jmacd it is not necessary about fewer allocation but also that a system (or external) call like (getting memory) may return data for more than one bound which means that if a callback per-bound is allocated the system call will be done multiple times. In case multiple system (or external) calls are required the callback can deal with that and update multiple bounds. As an example see https://github.com/open-telemetry/opentelemetry-java/blob/master/contrib/runtime_metrics/src/main/java/io/opentelemetry/contrib/metrics/runtime/GarbageCollector.java#L59 where I pre-allocate a list of bounds and update all of them after doing only one system/external call. |
|
Please see Bogdan's example (also linked above): |
|
@bogdandrutu @c24t @yurishkuro please take another look. |
| convention is not needed for Observer instruments because there is | ||
| little if any performance benefit in doing so (as Observer instruments | ||
| are called during collection, there is no need to maintain "active" | ||
| records concurrent with collection). |
There was a problem hiding this comment.
Bounding an instrument may involve encoding the label set into a wire-ready data structure, which in itself can be expensive if it needs to be done repeatedly on every measurement.
There was a problem hiding this comment.
The LabelSet value is meant to capture any encoding of label sets into wire-ready data structures, since these can be used for more than one bound instrument. In the example snippet, the label set is re-used to emphasize this. The only optimization that I can see would be if the Observer were to output more than once for the same LabelSet per collection period, in which case binding them would support the optimization.
| private static final ObserverDouble cpuLoad = ...; | ||
|
|
||
| void init() { | ||
| LabelSet labelSet = meter.createLabelSet("low_power", isLowPowerMode()); |
There was a problem hiding this comment.
compile error, missing final :-)
This is a clarification of the format OTEP 0008. This topic encountered last-minute debate in open-telemetry/opentelemetry-specification#250 and was left out. The section on calling conventions and examples here should clarify the issue.