open-telemetry
Add remove method to synchronous instruments
Fixes #2232
Changes
Add the ability to remove a synchronous instrument identified by its attributes. The instrument will no longer report.
One thing that has been asked for is to be able to delete multiple series at once, since callers often don't have access to the complete list of attribute sets they've previously incremented. E.g. remove(http.target=foo) would remove all streams for that http.target. One way to solve this is by matching all attribute sets which don't have the keys provided. Remove without any arguments would match and remove all streams from the instrument. Unfortunately, it wouldn't be backwards-compatible to change this later, so we need to decide if this is important from the start.
The other big question (which we need to resolve in the SDK spec PR), is how this impacts start time handling for cumulative metrics in the SDK. If an attribute set is deleted and recreated, the resulting metric data must have non-overlapping start-end time ranges since the cumulative value has been (presumably) reset. One way to solve this would be to require per-attribute-set start times in the SDK (https://github.com/open-telemetry/opentelemetry-specification/issues/4184).
A more flexible version of @dashpole's suggestion might look like: remove(Predicate<Attribute> predicate), where the predicate is invoked for each series, with usage in java like:
instrument.remove(attributes -> true); // Remove all series
instrument.remove(attributes -> attribute.get("http.route").equals("/v1/foo/bar")); // Remove all series where http.route=/v1/foo/bar
instrument.remove(attributes -> attributes.get("http.route").startsWith("/v1/foo")); // Remove all series matching pattern http.route=/v1/foo.*
If an attribute set is deleted and recreated, the resulting metric data must have non-overlapping start-end time ranges since the cumulative value has been (presumably) reset. One way to solve this would be to require per-attribute-set start times in the SDK (https://github.com/open-telemetry/opentelemetry-specification/issues/4184).
Yeah for example, in java, we always use the same SDK start time for all cumulative series. Whether they see their first data at start time or days later, the start is always the same. I think your suggestion to track per-attribute-set start times in the SDK seems reasonable, but that seems to imply a behavior change from the single constant start time that we currently do java. Are there implications for this? Are there cases where a user with a cumulative backend would be upset to see new series with a start time corresponding to the window where data was first recorded?
Also, if I call something like instrument.remove(attributes -> true) to delete all series, its not clear whether I intend to record additional data in the future. If I don't intend to record additional data, then I would probably view the fact that the SDK continues to have memory allocated to the instrument as a memory leak. But the SDK can't free up all the resources for that instrument without knowing for sure that I won't record again.
Makes me wonder if we need a top level instrument level close / remove method, as well as the fine grained method for removing specific series.
Are there implications for this? Are there cases where a user with a cumulative backend would be upset to see new series with a start time corresponding to the window where data was first recorded?
I don't think it will impact Prometheus in any negative way (@ArthurSens might know more, since he opened the original issue).
Depending on how strict you want to be, it may make it harder to aggregate timeseries with different start timestamps. If you user the earliest start timestamp, you may be missing data, and not produce an accurate cumulative for the entire time range.
Makes me wonder if we need a top level instrument level close / remove method, as well as the fine grained method for removing specific series.
This can be added later and separately from this effort, from what I can tell.
If an attribute set is deleted and recreated, the resulting metric data must have non-overlapping start-end time ranges since the cumulative value has been (presumably) reset. One way to solve this would be to require per-attribute-set start times in the SDK (https://github.com/open-telemetry/opentelemetry-specification/issues/4184).
My concrete use case is tied to a queue system where we report the count of events seen. See https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/ibm-mq-metrics/model/metrics.yaml#L216
When we lose contact with the queue manager, we can no longer report this information. If we get back in contact with the queue manager, we must recreate the time series with the new information, resetting the counter and its start time. We typically alert on delta changes, so we want the time series to be separate.
we must recreate the time series with the new information, resetting the counter and its start time. We typically alert on delta changes, so we want the time series to be separate.
How do you reset the start time? All SDKs i'm aware of in OTel today set the cumulative start time at instrument creation time, and never reset it.
Are there implications for this? Are there cases where a user with a cumulative backend would be upset to see new series with a start time corresponding to the window where data was first recorded?
Actually, we would be happier to see this :) I've opened https://github.com/open-telemetry/opentelemetry-specification/issues/4184 a long time ago, but never found the time to continue working on it. A start time per time series would help us provide more accurate increase rates.
Since this is now sponsored, I am marking this PR ready for review. The discussion continues.
we must recreate the time series with the new information, resetting the counter and its start time. We typically alert on delta changes, so we want the time series to be separate.
How do you reset the start time? All SDKs i'm aware of in OTel today set the cumulative start time at instrument creation time, and never reset it.
Let's put this in as a requirement and try it out in the POCs, see how we fare.
This PR was marked stale due to lack of activity. It will be closed in 7 days.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@atoulme
My concrete use case is tied to a queue system where we report the count of events seen. See https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/ibm-mq-metrics/model/metrics.yaml#L216
When we lose contact with the queue manager, we can no longer report this information. If we get back in contact with the queue manager, we must recreate the time series with the new information, resetting the counter and its start time. We typically alert on delta changes, so we want the time series to be separate.
This issue has been raised and resisted a number of times in OpenTelemetry. I understand there is still an unanswered need, but I do not like the verb remove as the API method name, it's not clearly "removing" anything; it's not de-registering the instrument. We are not trying to erase the memory of the instrument, we're trying to get series out of memory. We need to report final measurements, "seal" the timeseries in some manner, and then forget about the data. If I could choose the verb for this action, it's "finish", it means "flush and forget". I think the idea of passing a predicate to select series for finishing makes sense.
Consumers should receive the correct finalized value of these series. As @ArthurSens points out in #4184, what we need is a specification for how the data should be transmitted so that the ending of a series is clear. In Prometheus, we have the NaN value, and in OTel we have the missing data point flag, but we've never specified how to set that flag. I would like to see a specification that dictates SDKs have to remember the "finishing" series long enough to send the NaN/missing-data-flag to each reader at least once, otherwise that reader would lose information. Then, to answer #4184, we need to specify that new series must be created with a start time >= the Nan/missing-data-flag previously issued for the same series.
This PR was marked stale due to lack of activity. It will be closed in 7 days.
Closed as inactive. Feel free to reopen if this PR is still being worked on.
@atoulme
My concrete use case is tied to a queue system where we report the count of events seen. See https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/ibm-mq-metrics/model/metrics.yaml#L216
When we lose contact with the queue manager, we can no longer report this information. If we get back in contact with the queue manager, we must recreate the time series with the new information, resetting the counter and its start time. We typically alert on delta changes, so we want the time series to be separate.
This issue has been raised and resisted a number of times in OpenTelemetry. I understand there is still an unanswered need, but I do not like the verb
removeas the API method name, it's not clearly "removing" anything; it's not de-registering the instrument. We are not trying to erase the memory of the instrument, we're trying to get series out of memory. We need to report final measurements, "seal" the timeseries in some manner, and then forget about the data. If I could choose the verb for this action, it's "finish", it means "flush and forget". I think the idea of passing a predicate to select series for finishing makes sense.
Sure, finish works.
Consumers should receive the correct finalized value of these series. As @ArthurSens points out in #4184, what we need is a specification for how the data should be transmitted so that the ending of a series is clear. In Prometheus, we have the NaN value, and in OTel we have the missing data point flag, but we've never specified how to set that flag. I would like to see a specification that dictates SDKs have to remember the "finishing" series long enough to send the NaN/missing-data-flag to each reader at least once, otherwise that reader would lose information. Then, to answer #4184, we need to specify that new series must be created with a start time >= the Nan/missing-data-flag previously issued for the same series.
Are you offering to work on that specification? Is it a requirement for this or a follow-up?
We need to report final measurements, "seal" the timeseries in some manner, and then forget about the data.
In my use case we have the scenario where we report metrics while some external object is 'alive'. When an object is dead we want to purge attributes since it would just leak memory. But that said object could come back if say the user creates it so we would start reporting metrics for it again.
For example Knative has an Activator component and we report metrics using a Revision name/namespace attributes. Revision names come and go.
@jmacd - So I'm not sure if 'seal' is right because that to me makes the time series immutable - was that intentional?
I like what @dashpole suggests here https://github.com/open-telemetry/opentelemetry-specification/pull/4702#discussion_r2499279452 we're actually 'unregistering' attributes
Also I think not stressing about the name is important. We should focus and move this along. Users are reporting memory in Knative leaks because of the lack of this feature :/