steffan-westcott / clj-otel

An idiomatic Clojure API for adding telemetry to your libraries and applications using OpenTelemetry.
https://cljdoc.org/d/com.github.steffan-westcott/clj-otel-api
Apache License 2.0
183 stars 12 forks source link

Async instrument API does not allow recording multiple measurements #5

Closed loganlinn closed 1 year ago

loganlinn commented 1 year ago

Problem

Proposal

I see two potential approaches that would move this forward without introducing breaking changes to the current API:

  1. Allow observe to return a sequence of maps, each corresponding to a underlying call to record.
    • The current callback code would inspect value returned by observe using map? and/or sequential?.
    • Note: this would happen each time async instrument is observed.
  2. (preferred) Introduce a new protocol that could be used for the 2nd argument of instrument instead of a function.

    • This protocol could look like:

      (defprotocol Observable
        (observe! [this record!]))
    • The record! argument is a function that either:

      1. Accepts a single argument that is a map similar to the return value of the current observe API: :value key + optional :attributes key, OR
      2. (preferred) Has two arities to accept these values directly, i.e

        (fn record!
          ([value] ,,,)
          ([value attributes] ,,,))
    • The steffan-westcott.clj-otel.api.metrics.instrument/instrument function would detect opt-in via (satisfies? Observable observe).

    • The steffan-westcott.clj-otel.api.metrics.instrument namespace could provide helper function to reify this protocol to reduce burden on end-user.

My preference for the 2nd approach, despite being a little more involved, is for performance/efficiency reasons. While I am generally sensitive to premature optimization, using values like {:value ..., :attributes ...} to convey the arguments to OpenTelemetry API invocations does introduce objects that must be garbage collected at some point. As a end-user who wants to instrument my application code, I want to incur minimal overhead in doing so. It would be fairly trivial for an and end-user who does prefer a data-oriented approach to accomplish the same effect because the record! function can be wrapped.

steffan-westcott commented 1 year ago

Thank you for raising this issue. On closer examination, I agree the current design is inflexible compared to the Java API. Using the Java API, the callback could contain arbitrary logic and dynamically record any number of measurements. I will remove the 0-arity observe function and replace it with something closer to the ObservableMeasurement Java interface.

As Metrics API support is new for the next clj-otel release, I am not concerned with breaking changes in this area.

steffan-westcott commented 1 year ago

I implemented the first suggestion of allowing observe to return a sequence of maps.

Implementing the second suggestion is difficult, as the ObservableMeasurement instance is known only at observation time, not at instrument construction time. Constructing a record! function instance on each observation is possible, but this is counterproductive to minimising garbage collection.

loganlinn commented 1 year ago

@steffan-westcott That makes complete sense now. Thank you for considering both and the quick turn around!