[internal/exp/metrics] Add a new internal package for handling metric staleness #31089
Conversation
RichieSams
force-pushed
the
exp/metrics/staleness
branch
from
18aa7c3 to
e12c0ff
Compare
RichieSams
force-pushed
the
exp/metrics/staleness
branch
from
6bea8be to
4f9ab5c
Compare
|
@djaglowski @jpkrohling @fatsheep9146 @bryan-aguilar This PR is ready for final review. (Can one of you add the "Skip Changelog" label, since this is an internal-only change) @sh0rez Any final comments? |
| var _ Map[time.Time] = (*RawMap[identity.Stream, time.Time])(nil) | ||
|
|
||
| // RawMap is an implementation of the Map interface using a standard golang map | ||
| type RawMap[K comparable, V any] map[K]V |
There was a problem hiding this comment.
I think this type doesn't creeps a bit beyond the scope of this package.
I like that this package defines the Map[T] interface as the behavior it expects to fulfill its purpose.
A general stream map would feel more fitting as a streams.HashMap or similar.
I'd be okay with this pkg not defining a map implementation at all, leaving that to consumers.
No hard opinion however, if you prefer we can leave it in here for now, as we'll likely iterate on these packages for a bit anyways.
There was a problem hiding this comment.
I'd be okay with this pkg not defining a map implementation at all, leaving that to consumers.
That was my initial plan. I had implemented RawMap just in the tests. But then I realized that I would need to re-implement the same code in the interval processor (and perhaps you as well in deltatocumulative). So I moved it to be in the package proper as a convenience "reference" implementation.
But I'm kind of the same. I don't feel strongly either way
There was a problem hiding this comment.
Are you ok with leaving it in for now? And iterating as the various processors utilize it?
RichieSams
force-pushed
the
exp/metrics/staleness
branch
from
4f9ab5c to
a3d08a8
Compare
There was a problem hiding this comment.
LGTM but I will wait to approve until @sh0rez's feedback is fully addressed.
… staleness It's a glorified wrapper over a Map type, which allows values to be expired based on a pre-supplied interval.
So Staleness isn't so tightly tied to it
So users are forced to use the correct methods. Also adds lots of documentation
RichieSams
force-pushed
the
exp/metrics/staleness
branch
from
cde1ec7 to
d0f1d14
Compare
|
@djaglowski ready to merge from my side! Think this still needs the "no changelog" label |
) **Description:** Removes stale series from tracking (and thus frees their memory) using staleness logic from open-telemetry#31089 **Link to tracking Issue:** open-telemetry#30705, open-telemetry#31016 **Testing:** `TestExpiry` **Documentation:** README updated
… staleness (open-telemetry#31089) **Description:** It's a glorified wrapper over a Map type, which allows values to be expired based on a pre-supplied interval. **Link to tracking Issue:** open-telemetry#31016 **Testing:** I added some basic tests of the PriorityQueue implementation as well as the expiry behaviour of Staleness **Documentation:** All the new structs are documented
) **Description:** Removes stale series from tracking (and thus frees their memory) using staleness logic from open-telemetry#31089 **Link to tracking Issue:** open-telemetry#30705, open-telemetry#31016 **Testing:** `TestExpiry` **Documentation:** README updated
Description:
It's a glorified wrapper over a Map type, which allows values to be expired based on a pre-supplied interval.
Link to tracking Issue:
#31016
Testing:
I added some basic tests of the PriorityQueue implementation as well as the expiry behaviour of Staleness
Documentation:
All the new structs are documented