Metric Observer - change from callback to Observable

[obecny]

Currently Metric Observer is using callback to get a value. The callback is called periodically, and is returning the value. This is causing the problem to be able to update value in asynchronous operation. If we change the callback to Observable we will be able to set the value at any given time. It would look then more less like this

    const observable1 = new Observable();
    const meter = new MeterProvider({
      interval: 30000,
      exporter: someExporter,
    }).getMeter('test');
    const observer = meter.createObserver('test', {});
    observer.setCallback((observerResult) => {
      observerResult.observe(observable1, { name: 'foo1' });
    });
    
    observable1.next(1);
    // or
    setTimeout(()=> {
      observable1.next(5);
    }, 300);
    // or
    somePromise().then((value)=> {
      observable1.next(value);
    })

WDYT ?

[dyladan]

Are you thinking to implement Observable or import some Observable library?

[obecny]

I would implement the basic Observable, no need for 3rd party lib

[obecny]

and then the code for ObserverMetric.setCallback could be similar to something like this (more less to just give you the rough idea)

  setCallback(callback: (observerResult: types.ObserverResult) => void): void {
    callback(this._observerResult);
    this._observerResult.observers.forEach((observable, labels) => {
      observable.subscribe((value)=> {
        const instrument = this.bind(labels);
        instrument.update(value);
      })
    });
  }

[obecny]

in fact we could have both callback if someone needs just a sync operation (as it is now) or observer for any async operations

[obecny]

@mayurkale22 && @open-telemetry/javascript-approvers would like to hear your thoughts on that, thx

[vmarchaud]

Looks fine to me 👍

[dyladan]

Can we do this and still remain compliant to the spec? I want to stay as similar to other sigs as possible

[obecny]

the spec says it should be callback - for sync operation we can have like this, but for async I don't see so far any easy way to be able to do it. So we can easily support 2 cases. The biggest advantage will be that we can support async operation without trying to fix the problem of waiting for async to finish and the time of lastUpdate will be correct too. Besides that the change won't be so big too

[dyladan]

I think it's a good idea let's go for it

[dyladan]

@obecny were you thinking you wanted to take this?

[mayurkale22]

So this will change only internals of the implementations and not on API side, (No breaking change) right?

[obecny]

@dyladan yes as I'm working on metrics now anyway, so that's the whole idea where it comes from

[obecny]

@mayurkale22 we will have to extend our metrics to allow for callback and for Observable

[obecny]

@mayurkale22 here ->

opentelemetry-js/packages/opentelemetry-api/src/metrics/ObserverResult.ts

Line 24 in 3d26f82

observe(callback: Function, labels: Labels): void;

[dyladan]

@mayurkale22 it is backwards compatible change for metrics as it should just add one additional type accepted by the observerResult.observe method right @obecny?

Currently we have

function getCpuUsage() {
  return Math.random();
}

otelCpuUsage.setCallback((observerResult) => {
  observerResult.observe(getCpuUsage, { pid: process.pid });
});

We wil extend it to allow:

function getDiskUsage() {
  return Math.random();
}

const diskObservable = new MetricObservable();

// update disk usage every 10 seconds
setInterval(() => diskObservable.next(getDiskUsage()), 10_000)

otelDiskUsage.setCallback((observerResult) => {
  observerResult.observe(diskObservable, { disk: 0 });
});

[obecny]

@dyladan correct

[obecny]

But the whole idea is to allow for something like this

const diskObservable = new MetricObservable();
function getDiskUsage() {
  someAsyncPromiseEtc().then((value)=>{
    diskObservable.next(value);
  })
}

// update disk usage every 10 seconds
setInterval(getDiskUsage, 10_000)

otelDiskUsage.setCallback((observerResult) => {
  observerResult.observe(diskObservable, { disk: 0 });
});

[dyladan]

Right, I just thought the sync example was easier to read for the example. We're on the same page :)

[dyladan]

observe(callback: Function, labels: Labels): void; signature will change to observe(callback: Function | MetricObservable, labels: Labels): void; which is a backwards compatible change.