Add mostrecent aggregation to Gauge #967
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
draftcode
force-pushed
the
most_recent_aggregation
branch
2 times, most recently
from
95820f7 to
cc84ce2
Compare
draftcode
force-pushed
the
most_recent_aggregation
branch
2 times, most recently
from
172733f to
27ca983
Compare
|
OK. Addressed the comments. Please take a look again. |
draftcode
force-pushed
the
most_recent_aggregation
branch
from
27ca983 to
831a5b6
Compare
draftcode
force-pushed
the
most_recent_aggregation
branch
from
831a5b6 to
5d946a5
Compare
There was a problem hiding this comment.
The code looks good, thanks! I realized we should also update the "Metrics tuning (Gauge):" section in the README before merging, basically just adding a bullet point for 'recent'.
In the multiprocess mode, the process that exposes the metrics needs to aggregate the samples from other processes. Gauge metric allows users to choose the aggregation mode. This implements 'mostrecent' (and 'livemostrecent') mode where the last observed value is exposed. In order to support this, the file format is expanded to store the timestamps in addition to the values. The stored timestamps are read by the reader process and it's used to find the latest value. Closes prometheus#847 Consideration on the atomicity: Previously, mmap_dict.py had a comment saying "We assume that reading from an 8 byte aligned value is atomic". With this change, the value write becomes a 16 bytes 8-byte aligned write. The code author tried to find a basis on the original assumption, but couldn't find any. According to write(2), **if a file descriptor is shared**, the write becomes atomic. However, we do not share the file descriptors in the current architecture. Considering that Ruby implementation also does the same and hadn't seen an issue with it, this write atomicity problem might be practically not an issue. See also: * prometheus/client_ruby#172 The approach and naming are taken from client_ruby. * https://github.com/prometheus/client_golang/blob/v1.17.0/prometheus/metric.go#L149-L161 client_golang has an API for setting timestamp already. It explains the use case for the timestamp beyond the client-local aggregation. In order to support the same use case in Python, further changes are needed. Signed-off-by: Masaya Suzuki <[email protected]>
draftcode
force-pushed
the
most_recent_aggregation
branch
from
5d946a5 to
20ade3c
Compare
Done. |
jvansanten
added a commit
to AmpelProject/Ampel-core
that referenced
this pull request
Dec 21, 2023
prometheus/client_python#967 jiggers with the format of multiprocess metrics in a way that breaks our custom aggregation. Ignore for now.
jvansanten
added a commit
to AmpelProject/Ampel-core
that referenced
this pull request
Jan 8, 2024
prometheus/client_python#967 jiggers with the format of multiprocess metrics in a way that broke our custom aggregation.
jvansanten
added a commit
to AmpelProject/Ampel-core
that referenced
this pull request
Jan 8, 2024
prometheus/client_python#967 jiggers with the format of multiprocess metrics in a way that broke our custom aggregation.
jvansanten
added a commit
to AmpelProject/Ampel-core
that referenced
this pull request
Jan 8, 2024
* fix(deps): update minor updates * fix: support timestamps in prometheus 0.18 prometheus/client_python#967 jiggers with the format of multiprocess metrics in a way that broke our custom aggregation. --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Jakob van Santen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the multiprocess mode, the process that exposes the metrics needs to aggregate the samples from other processes. Gauge metric allows users to choose the aggregation mode. This implements 'mostrecent' (and 'livemostrecent') mode where the last observed value is exposed.
In order to support this, the file format is expanded to store the timestamps in addition to the values. The stored timestamps are read by the reader process and it's used to find the latest value.
Closes #847
Consideration on the atomicity:
Previously, mmap_dict.py had a comment saying "We assume that reading from an 8 byte aligned value is atomic". With this change, the value write becomes a 16 bytes 8-byte aligned write. The code author (draftcode) tried to find a basis on the original assumption, but couldn't find any. According to write(2), if a file descriptor is shared, the write becomes atomic. However, we do not share the file descriptors in the current architecture.
Considering that Ruby implementation also does the same and hadn't seen an issue with it, this write atomicity problem might be practically not an issue.
See also:
Add :most_recent aggregation to DirectFileStore client_ruby#172
The approach and naming are taken from client_ruby.
https://github.com/prometheus/client_golang/blob/v1.17.0/prometheus/metric.go#L149-L161
client_golang has an API for setting timestamp already. It explains the use case for the timestamp beyond the client-local aggregation. In order to support the same use case in Python, further changes are needed.