Consider renaming observable callback return type from Measurement -> Observation

[aabmass]

opentelemetry-python/opentelemetry-api/src/opentelemetry/_metrics/measurement.py

Line 20 in fd9862a

class Measurement:

To differentiate this from the SDK's internal Measurement class, we could rename this class to Observation, as it is the thing returned from observable instruments' callback methods. Complete example of an observable instrument:

def cpu_time_callback() -> Iterable[Observation]:
    with open("/proc/stat") as procstat:
        procstat.readline()  # skip the first line
        for line in procstat:
            if not line.startswith("cpu"): break
            cpu, *states = line.split()
            yield Observation(int(states[0]) // 100, {"cpu": cpu, "state": "user"})
            yield Observation(int(states[1]) // 100, {"cpu": cpu, "state": "nice"})
            # ... other states

meter.create_observable_counter(
    "system.cpu.time",
    callback=cpu_time_callback,
    unit="s",
    description="CPU time"
)

Thoughts?

[ocelotl]

Hm, another question first, why do we have an implementation of the Measurement class in the API? Both the SDK and API Measurement classes have the same attributes.

🤔

Should we have an abstract Measurement class in the API and a concrete one in the SKD instead?

[aabmass]

They are more or less unrelated, just share the same name, which is why I think renaming the Observable callback one to Observation would be cleaner

[aabmass]

We will probably need to rework this API regardless to handle multi-instrument callbacks. See open-telemetry/opentelemetry-specification#2317 (comment)

[lzchen]

@aabmass
Renaming the api class to Observation sounds good to me.

[ocelotl]

They are more or less unrelated, just share the same name, which is why I think renaming the Observable callback one to Observation would be cleaner

I don't disagree, I am just curious why do we have an implementation in the API 🤷

If we decide to have only an implementation in the SDK, should we have there two classes with the same attributes but different names?

Thoughts, @aabmass, @lonewolf3739, @lzchen ?

[srikanthccv]

What do you mean when you say "why do we have an implementation in the API"? If there was no such class Measurement/Observation how do we expect library authors to instrument their code just with api component?

[ocelotl]

What do you mean when you say "why do we have an implementation in the API"? If there was no such class Measurement/Observation how do we expect library authors to instrument their code just with api component?

This is an enlightening question 👍

The idea that I have of the API (and thinking about error handling lead me into this idea) is that it contains 4 things:

1. A set of abstract classes.
2. A set of proxy classes that matches the set in point 1.
3. An implementation of the previous set of abstract classes in point 1. These concrete classes are all NoOp* classes (yes, I actually mean the API contains an implementation of itself, and this implementation is in this sense analogous to the SDK).
4. Specific concrete classes that are required by the spec.

In the case of Measurement here, if we followed this idea, what we would need is an abstract Measurement class and a NoOpMeasurement class as well. To answer @lonewolf3739 question, library authors would instrument their code with the NoOpMeasurement class as they would do normally with the rest of NoOp* classes.

Now, Measurement has very little functionality if any (pretty much the only method that does "something" is __eq__), so what would a NoOpMeasurement class be?

I think for consistency with this idea, a NoOpMeasurement class should still be in the API, and may do exactly what the Measurement class now does or its __eq__ method may always return True or its attributes would always be None or something else. It is something to think about 👍

[aabmass]

This falls under 4, it is a concrete class in the API that is used as part of instrumentation. It is not in the spec explicitly because the spec is very flexible. Here's the API spec for this:

The callback function is responsible for reporting the Measurements. It will only be called when the Meter is being observed. OpenTelemetry API authors SHOULD define whether this callback function needs to be reentrant safe / thread safe or not.

[...]

OpenTelemetry API authors MAY decide what is the idiomatic approach. Here are some examples:

• Return a list (or tuple, generator, enumerator, etc.) of Measurements.
• Use an observable result argument to allow individual Measurements to be reported.

[...]

def ws_callback():
    # Note: in the real world these would be retrieved from the operating system
    return (
        (8,      ("pid", 0),   ("bitness", 64)),
        (20,     ("pid", 4),   ("bitness", 64)),
        (126032, ("pid", 880), ("bitness", 32)),
    )

Instead of a tuple of value and attributes, we provide Measurement as a named class to use instead.

[ocelotl]

We will probably need to rework this API regardless to handle multi-instrument callbacks. See open-telemetry/opentelemetry-specification#2317 (comment)

Is the motivation for this change related to this topic? I don't necessarily disagree with having a class named Measurement and another one named Observation, but I would like to understand what reason motivates us to have different classes. Is there something that needs to use the class type to make a decision?

[ocelotl]

As mentioned in the SIG, the SDK Measurement will have an additional context attribute, so they are different. I am ok with this change, then 👍

[lzchen]

@aabmass @ocelotl
Apologies if this has been already discussed. I agree with the renaming of Measurement -> Observation to represent what is returned from asynch instruments.

Should we move actual Measurement to api since it is defined in the api specs?

[ocelotl]

@aabmass @ocelotl Apologies if this has been already discussed. I agree with the renaming of Measurement -> Observation to represent what is returned from asynch instruments.

Should we move actual Measurement to api since it is defined in the api specs?

If I remember correctly, we have found situations before where it was ok that something defined in the api document was implemented in the sdk document, so I think it is ok to have that in the api or in the sdk.