feat: remove CR meters when they are deleted (after a delay)#1805
feat: remove CR meters when they are deleted (after a delay)#1805
Conversation
|
I guess the target should be the |
|
Yes |
metacosm
force-pushed
the
metrics-cleaning
branch
from
15cd69c to
46a674a
Compare
|
|
||
| public MicrometerMetrics(MeterRegistry registry, int cleanUpDelayInSeconds) { | ||
| this.registry = registry; | ||
| this.cleanUpDelayInSeconds = cleanUpDelayInSeconds; |
There was a problem hiding this comment.
Why is there a delay, why cannot be deleted instantly?
There was a problem hiding this comment.
Because you still might want to get the metrics on the deleted resources for a while before the metrics are removed.
There was a problem hiding this comment.
true, would worth to write it as a javadoc so it is obvious from code.
There was a problem hiding this comment.
Yes, the feature needs to be documented.
| private final MeterRegistry registry; | ||
| private final Map<String, AtomicInteger> gauges = new ConcurrentHashMap<>(); | ||
| private final Map<ResourceID, Set<Meter.Id>> metersPerResource = new ConcurrentHashMap<>(); | ||
| private final ScheduledExecutorService metersCleaner = Executors.newScheduledThreadPool(10); |
There was a problem hiding this comment.
Since the clean is a short running task (I assume), wouldn't' be better to use a Timer?
There was a problem hiding this comment.
Because it's usually better to use ScheduledExecutorService than Timer. See https://stackoverflow.com/a/409993.
There was a problem hiding this comment.
Ok, then the number of Threads should be configurable, and we should use 1 as default.
There was a problem hiding this comment.
Actually the whole ScheduledExecutorService should be configurable.
There was a problem hiding this comment.
Both delay and thread numbers are configurable. What other aspects would need configuration?
There was a problem hiding this comment.
For example bursting the threads (like the cached executor), but I think it fine for now now
| incrementCounter(resourceID, "events.delete", metadata); | ||
|
|
||
| // schedule deletion of meters associated with ResourceID | ||
| metersCleaner.schedule(() -> { |
There was a problem hiding this comment.
Since this is a non trivial logic, we should add unit test.
There was a problem hiding this comment.
I'm not sure a unit test makes sense here, though I certainly could add one. What would make more sense would be an integration test to check that the metrics are actually not there anymore, will see how complex that could be to set up.
There was a problem hiding this comment.
IMO we should tests every non trivial logic with unit tests, and see at least that the API there is called for every ID eventually.
There was a problem hiding this comment.
The logic is trivial: on delete, call remove on all the meters associated with the resource id. I've added an integration test, anyway.
metacosm
force-pushed
the
metrics-cleaning
branch
from
238df1e to
f2c7d9c
Compare
metacosm
force-pushed
the
metrics-cleaning
branch
from
f2c7d9c to
d9c27e2
Compare
|
Kudos, SonarCloud Quality Gate passed! |
csviri
left a comment
csviri
left a comment
There was a problem hiding this comment.
Made one comment on default otherwise looks good to me
...pport/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java
Outdated
Show resolved
Hide resolved
* feat: remove CR meters when they are deleted (after a delay) Fixes #1803. Also added documentation for the Metrics feature.
* feat: remove CR meters when they are deleted (after a delay) Fixes #1803. Also added documentation for the Metrics feature.
* feat: remove CR meters when they are deleted (after a delay) Fixes #1803. Also added documentation for the Metrics feature.
2 participants
Fixes #1803.