[processor/deltatocumulative] limit tracked streams #31488
Conversation
|
I realize this is still draft, and you're iterating. But when everything is ready, can we split the |
sh0rez
force-pushed
the
deltatocumulative-limits
branch
from
2a9f804 to
181f34a
Compare
|
This self-simplified a lot with the merge of the recent staleness PR. Its ready for final review @RichieSams @djaglowski @ethercrow |
|
Just a small comment. Otherwise, LGTM |
| if err := m.Map.Store(id, v); err != nil { | ||
| return err | ||
| } | ||
| return ErrEvicted{ErrLimit: errl, id: gone} |
There was a problem hiding this comment.
Should this return an error? Yes we had to evict something, but the store itself was a "success". IMO, these aren't actual errors. Just statistics
There was a problem hiding this comment.
at this point they are errors imo, the consumer (processor) should decide what to do with them.
when #31363 is merged, those will turn into metrics.
Until then I think they are best treated as errors
There was a problem hiding this comment.
Sure. But I guess my point is that eviction itself isn't an error. It's just something that happens. IE, eviction should not increment the metrics_processed{error=true} metric (I don't recall the correct name :P ).
Perhaps the eviction yes/no value should be returned as an additional return, instead of being convolved with the error status. Thoughts?
There was a problem hiding this comment.
I agree eviction should not increment that specific counter, but rather be streams_evicted, so you can easily alert on that. Imo eviction should never be part of normal operations, as it signals a fairly serious capacity issue and thus is very close to an error scenario.
I think it's a not uncommon thing in Go to use the Error for non-fatal occurrences, as that's the whole point of having a value-based, easy to manipulate error system.
|
I'm merging without final approval from @djaglowski as he's not available this week, and I believe there's nothing here that can't be worked on via follow-up PRs. |
**Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #31488 was recently merged and has broken `build-and-test` for all builds due to a lint failure. It looks like two PRs were worked on in parallel, the aforementioned one, as well as #31625. #31625 renamed `exp` to `stale`, but the most recently merged PR was referencing the original `exp` variable. This is an unreleased component, and is simply fixing a bug, so I don't think this should have a changelog.
…1488) **Description:** Adds a configurable upper limit to the number of tracked streams. This allows to introduce a upper bound to the memory usage. **Testing:** Test case was added **Documentation:** README was updated
**Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> open-telemetry#31488 was recently merged and has broken `build-and-test` for all builds due to a lint failure. It looks like two PRs were worked on in parallel, the aforementioned one, as well as open-telemetry#31625. open-telemetry#31625 renamed `exp` to `stale`, but the most recently merged PR was referencing the original `exp` variable. This is an unreleased component, and is simply fixing a bug, so I don't think this should have a changelog.
…1488) **Description:** Adds a configurable upper limit to the number of tracked streams. This allows to introduce a upper bound to the memory usage. **Testing:** Test case was added **Documentation:** README was updated
**Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> open-telemetry#31488 was recently merged and has broken `build-and-test` for all builds due to a lint failure. It looks like two PRs were worked on in parallel, the aforementioned one, as well as open-telemetry#31625. open-telemetry#31625 renamed `exp` to `stale`, but the most recently merged PR was referencing the original `exp` variable. This is an unreleased component, and is simply fixing a bug, so I don't think this should have a changelog.
Description: Adds a configurable upper limit to the number of tracked streams. This allows to introduce a upper bound to the memory usage.
Testing: Test case was added
Documentation: README was updated