Observable instruments #162

christiankjaer · 2023-03-30T09:25:32Z

Fixes #158
Fixes #159

@iRevive Before I work more on this, how do you think the API looks?

iRevive

Hey, thanks for the contribution!

I overlooked a few details regarding the callback signature while making a draft.

There are a few comments from my side regarding the abstraction. The direction looks good and promising!

iRevive · 2023-03-30T09:39:56Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/AsyncInstrumentBuilder.scala

+
+import cats.effect.Resource
+
+trait AsyncInstrumentBuilder[F[_], Input, Instrument[_[_], _]] {


I'm sorry, I've updated the scope of #158 recently since I spotted that we have ObservableInstrumentBuilder.

We can change it rather than adding a new AsyncInstrumentBuilder.

iRevive · 2023-03-30T10:00:50Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/AsyncInstrumentBuilder.scala

+
+  def withUnit(unit: String): Self
+  def withDescription(description: String): Self
+  def withCallback(cb: Instrument[F, Input] => F[Unit]): Self


Ah, I see. I missed the Java's signature while making a draft.

We can mimic Java API and add ObservableMeasurement:

trait ObservableMeasurement[F[_], A] { def record(value: A, attributes: attributes: Attribute[_]*): F[Unit] }

Then, the bulidWithCallback can be:

def buildWithCallback(cb: ObservableMeasurement[F, Input] => F[Unit]): Resource[F, Instrument[F, Input]]

iRevive · 2023-03-30T10:02:50Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/AsyncInstrumentBuilder.scala

+
+  def withUnit(unit: String): Self
+  def withDescription(description: String): Self
+  def withCallback(cb: Instrument[F, Input] => F[Unit]): Self


~~There should be no way to create an observable metric without a callback.~~

~~So we can merge withCallback and create:~~

def createWithCallback[A](cb: ObservableMeasurement[F, Input] => F[Unit]): Resource[F, Instrument[F, Input]]

Upd: already changed.

Haha. I did this before your comment :)

iRevive · 2023-03-30T10:04:12Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/ObservableGauge.scala

+
+trait ObservableGauge[F[_], A] {
+
+  def observe(value: A, attributes: Attribute[_]*): F[Unit]


According to Java's API, observable Gauge cannot 'observe' value on-demand, it picks up all values exclusively from the callback:

https://github.com/open-telemetry/opentelemetry-java/blob/4a8850cc64cb407a33eb2131a2ef9cbc44567dcf/api/all/src/main/java/io/opentelemetry/api/metrics/ObservableDoubleGauge.java

So we can keep the trait empty.

Yea. I kinda misused this already existing trait such that you get the instrument in the callback which intuitively makes sense to me.

christiankjaer · 2023-03-30T10:49:19Z

If this looks ok-ish, I will add the remaining instruments and try to test it. Also look at the macro stuff.

iRevive · 2023-03-30T12:52:45Z

Looks good so far

christiankjaer · 2023-03-30T16:25:56Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/ObservableCounter.scala

@@ -16,4 +16,4 @@

 package org.typelevel.otel4s.metrics

-trait ObservableCounter[F[_], A]
+trait ObservableCounter


Not sure what to do with these. It seems strange that they have these type parameters, when we really only use them in the callback. Now I tried to remove them, and that seems to aid in testing at least.

I've been thinking about this for a while and I tend to agree.

iRevive · 2023-03-31T09:20:46Z

java/metrics/src/test/scala/org/typelevel/otel4s/java/metrics/ObservableSuite.scala

+        .observableGauge("gauge")
+        .withUnit("unit")
+        .withDescription("description")
+        .createWithCallback(x => x.observe(42.0))


It would be worth adding an Attribute to. To make sure we propagate it downstream.

iRevive · 2023-03-31T09:23:07Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/Observable.scala

+
+import org.typelevel.otel4s.Attribute
+
+trait Observable[F[_], A] {


The Observable name could be too broad. Also, it may clash with java.util.Observable.

What if we use ObservableMeasurement there?

christiankjaer · 2023-04-01T09:52:02Z

I think this is good for a round of reviews.

iRevive

Looks good to me! I have a few minor suggestions regarding the documentation and that's it.

iRevive · 2023-04-03T10:50:21Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/Meter.scala

+          def withUnit(unit: String): Self = this
+          def withDescription(description: String): Self = this
+          def createWithCallback(
+              _cb: ObservableMeasurement[F, Double] => F[Unit]


A minor change:

Suggested change

_cb: ObservableMeasurement[F, Double] => F[Unit]

cb: ObservableMeasurement[F, Double] => F[Unit]

iRevive · 2023-04-03T10:51:02Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/ObservableCounter.scala

@@ -16,4 +16,4 @@

 package org.typelevel.otel4s.metrics

-trait ObservableCounter[F[_], A]
+trait ObservableCounter


I've been thinking about this for a while and I tend to agree.

iRevive · 2023-04-03T10:51:57Z

examples/src/main/scala/ObservableExample.scala

+import javax.management.MBeanServer
+import javax.management.ObjectName
+
+object ObservableExample extends IOApp.Simple {


Nice! Thanks for the relevant example.

iRevive · 2023-04-03T11:00:50Z

.../metrics/src/main/scala/org/typelevel/otel4s/java/metrics/ObservableCounterBuilderImpl.scala

+  def createWithCallback(
+      cb: ObservableMeasurement[F, Long] => F[Unit]
+  ): Resource[F, ObservableCounter] =
+    Dispatcher.parallel.flatMap(dispatcher =>


I'm wondering which dispatcher is more suitable. The initiator of the callback evaluation is OpenTelemtry internals (it's a MetricProducer#collectAllMetrics, I believe).

Should we guarantee the order?

I'm definitely not an expert on Dispatcher semantics. I'll just change this to sequential, since these callbacks are called quite rarely, so I don't think performance should be a worry.

I don't think it's going to matter a whole lot. My gut feeling is sequential is right.

iRevive · 2023-04-03T11:13:01Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/ObservableInstrumentBuilder.scala

  /** Creates an instrument with the given `unit` and `description` (if any).
    */


It's worth mentioning how the callback should behave. (mostly taken from OpenTelemetry Java doc)

Suggested change

/** Creates an instrument with the given `unit` and `description` (if any).

*/

/** Creates an instrument with the given callback, using `unit` and

* `description` (if any).

*

* The callback will be called when the instrument is being observed.

*

* The callback is expected to abide by the following restrictions:

* - Short-living and (ideally) non-blocking

* - Run in a finite amount of time

* - Safe to call repeatedly, across multiple threads

*

* @param cb

* The callback which observes measurements when invoked

*/

iRevive · 2023-04-03T11:15:54Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/ObservableMeasurement.scala

+import org.typelevel.otel4s.Attribute
+
+trait ObservableMeasurement[F[_], A] {
+  def record(value: A, attributes: Attribute[_]*): F[Unit]


Scaladoc:

Suggested change

def record(value: A, attributes: Attribute[_]*): F[Unit]

/** Records a value with a set of attributes.

*

* @param value

* the value to record

*

* @param attributes

* the set of attributes to associate with the value

*/

def record(value: A, attributes: Attribute[_]*): F[Unit]

iRevive · 2023-04-03T11:20:34Z

@rossabaker @zmccoy

Changing the type constraint from Sync to Async breaks binary compatibility in several places.
Since we are in 0.1 world and have already broken bincompat towards 0.2 by some other changes, should we merge it as is?

iRevive · 2023-04-03T11:22:06Z

core/metrics/src/main/scala/org/typelevel/otel4s/metrics/ObservableMeasurement.scala

+import org.typelevel.otel4s.Attribute
+
+trait ObservableMeasurement[F[_], A] {
+  def record(value: A, attributes: Attribute[_]*): F[Unit]


@christiankjaer it would be nice to preserve laziness via macro. It should be nearly identical to HistogramMacro.
Could you take a look there too?

Sure. The reason why I didn't look at the macro is that I kinda assumed that the observable stuff is for instrumenting from the outside, and the noop is then rarely needed. I will take a look.

observable stuff is for instrumenting from the outside

Good point. Since the callback is a function ObservableMeasurement[F, A] => F[Unit], it will never be executed if we use a noop implementation. So there's no need to worry about allocations.

So, we can keep it as-is and skip macro.

iRevive

Great work, thanks!

rossabaker

Yes, I think we're due for an 0.2 and have no problem with the broken compatibility. This is great work.

iRevive · 2023-04-10T17:22:32Z

@christiankjaer the docs/mdoc is failing since documentation uses the old methods: OtelJava.forSync instead of OtelJava.forAsync. Once the CI is green we can merge I believe.

christiankjaer · 2023-04-10T19:15:06Z

@christiankjaer the docs/mdoc is failing since documentation uses the old methods: OtelJava.forSync instead of OtelJava.forAsync. Once the CI is green we can merge I believe.

Should work now.

Observable instruments

iRevive reviewed Mar 30, 2023

View reviewed changes

christiankjaer commented Mar 30, 2023

View reviewed changes

iRevive reviewed Mar 31, 2023

View reviewed changes

christiankjaer changed the title ~~Prototype of observable gauge~~ Observable instruments Apr 1, 2023

Implementation of observable instruments

872534c

christiankjaer force-pushed the ckjaer/observable branch from d88c3cb to 872534c Compare April 1, 2023 09:14

christiankjaer marked this pull request as ready for review April 1, 2023 09:14

Added example

fcdc740

christiankjaer force-pushed the ckjaer/observable branch from 3e82e3f to fcdc740 Compare April 1, 2023 09:51

iRevive requested changes Apr 3, 2023

View reviewed changes

iRevive reviewed Apr 3, 2023

View reviewed changes

Review comments

c3ef24c

iRevive approved these changes Apr 5, 2023

View reviewed changes

iRevive requested review from rossabaker and zmccoy April 5, 2023 07:27

rossabaker approved these changes Apr 10, 2023

View reviewed changes

Fix example docs

c8c64f6

iRevive merged commit 7a95c3d into typelevel:main Apr 11, 2023

christiankjaer deleted the ckjaer/observable branch April 11, 2023 07:03

chr12c pushed a commit to chr12c/otel4s that referenced this pull request Jun 26, 2023

Merge pull request typelevel#162 from christiankjaer/ckjaer/observable

a81b29a

Observable instruments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Observable instruments #162

Observable instruments #162

christiankjaer commented Mar 30, 2023 •

edited

Loading

iRevive left a comment

iRevive Mar 30, 2023 •

edited

Loading

christiankjaer Mar 30, 2023

iRevive Mar 30, 2023 •

edited

Loading

iRevive Mar 30, 2023 •

edited

Loading

christiankjaer Mar 30, 2023

iRevive Mar 30, 2023 •

edited

Loading

christiankjaer Mar 30, 2023

christiankjaer commented Mar 30, 2023

iRevive commented Mar 30, 2023

christiankjaer Mar 30, 2023

iRevive Apr 3, 2023

iRevive Mar 31, 2023

iRevive Mar 31, 2023

christiankjaer commented Apr 1, 2023

iRevive left a comment

iRevive Apr 3, 2023

iRevive Apr 3, 2023

iRevive Apr 3, 2023

iRevive Apr 3, 2023

christiankjaer Apr 3, 2023

rossabaker Apr 10, 2023

iRevive Apr 3, 2023

iRevive Apr 3, 2023

iRevive commented Apr 3, 2023

iRevive Apr 3, 2023

christiankjaer Apr 3, 2023

iRevive Apr 3, 2023 •

edited

Loading

iRevive left a comment

rossabaker left a comment

iRevive commented Apr 10, 2023

christiankjaer commented Apr 10, 2023


		import cats.effect.Resource

		trait AsyncInstrumentBuilder[F[_], Input, Instrument[_[_], _]] {


		trait ObservableGauge[F[_], A] {

		def observe(value: A, attributes: Attribute[_]*): F[Unit]


		import org.typelevel.otel4s.Attribute

		trait Observable[F[_], A] {

	_cb: ObservableMeasurement[F, Double] => F[Unit]
	cb: ObservableMeasurement[F, Double] => F[Unit]

		/** Creates an instrument with the given `unit` and `description` (if any).
		*/

-  /** Creates an instrument with the given `unit` and `description` (if any).
-    */
+  /** Creates an instrument with the given callback, using `unit` and
+    * `description` (if any).
+    *
+    * The callback will be called when the instrument is being observed.
+    *
+    * The callback is expected to abide by the following restrictions:
+    *   - Short-living and (ideally) non-blocking
+    *   - Run in a finite amount of time
+    *   - Safe to call repeatedly, across multiple threads
+    *
+    * @param cb
+    *   The callback which observes measurements when invoked
+    */

-  def record(value: A, attributes: Attribute[_]*): F[Unit]
+  /** Records a value with a set of attributes.
+    *
+    * @param value
+    *   the value to record
+    *
+    * @param attributes
+    *   the set of attributes to associate with the value
+    */
+  def record(value: A, attributes: Attribute[_]*): F[Unit]

Observable instruments #162

Observable instruments #162

Conversation

christiankjaer commented Mar 30, 2023 • edited Loading

iRevive left a comment

Choose a reason for hiding this comment

iRevive Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

iRevive Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiankjaer commented Mar 30, 2023

iRevive commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiankjaer commented Apr 1, 2023

iRevive left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

iRevive left a comment

Choose a reason for hiding this comment

rossabaker left a comment

Choose a reason for hiding this comment

iRevive commented Apr 10, 2023

christiankjaer commented Apr 10, 2023

christiankjaer commented Mar 30, 2023 •

edited

Loading

iRevive Mar 30, 2023 •

edited

Loading

iRevive Mar 30, 2023 •

edited

Loading

iRevive Mar 30, 2023 •

edited

Loading

iRevive Mar 30, 2023 •

edited

Loading

iRevive Apr 3, 2023 •

edited

Loading