Monitoring improvements #616
Conversation
This probably isn't needed but it's nicer to clean after yourself :)
Type is an extended version of what was previously called ResourceAction. Since all events have types now, we can also remove CustomResourceEvent.
| @@ -4,4 +4,13 @@ | |||
|
|
|||
| CustomResourceID getRelatedCustomResourceID(); | |||
|
|
|||
| Type getType(); | |||
|
|
|||
| default boolean isDeleteEvent() { | |||
There was a problem hiding this comment.
I don't think this is actually a better model. Do I understand it correctly , that this is just to handle differentiation between delete and other events? The current way although it's not nicely abstracted, but it's quite descriptive.
There was a problem hiding this comment.
I'm not sure what the best way would be but having the event handler be aware of specific event types and have specific behavior to handle them is a sign that something is wrong with the implementation, in my opinion. I agree, though, that probably no other event sources will produce events with the type DELETED… so it is indeed also artificial to do it this way.
There was a problem hiding this comment.
I thing it was better before, since these events will be used also possible by users of the framework who implements an EventSource, so we should keep it as clean as possible for them.
...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java
Show resolved
Hide resolved
|
|
||
| public interface Metrics { | ||
| Metrics NOOP = new Metrics() {}; | ||
|
|
||
| default void processingEvent(Event event) {} |
There was a problem hiding this comment.
Is this really what we should measure? Isn't that the number of reconciliations in this case actually. Since we tried to move away from events.
There was a problem hiding this comment.
Yes, it measures the number of reconciliation requests. The thinking was that it might be interesting to see how many reconciliation requests happened vs. the number of reconciliations that actually went through (processedEvent).
There was a problem hiding this comment.
Shouldn't we rename the method accordingly? Like reconcilingStarted, reconcilingFinished, reconcilingFailed ?
|
|
||
| public ExecutionScope(R customResource, RetryInfo retryInfo) { | ||
| ExecutionScope(R customResource, RetryInfo retryInfo, Event triggeringEvent) { |
There was a problem hiding this comment.
This is actually meant before, do we really need this event? The idea was that from a certain layer we don't work with the events at all, not even in our internal classes, just with the marker. Note that usually it can be multiple event's that received and triggered the execution.
| @@ -280,7 +249,7 @@ private void handleRetryOnException(ExecutionScope<R> executionScope) { | |||
| if (eventPresent) { | |||
| log.debug("New events exists for for resource id: {}", | |||
| customResourceID); | |||
| submitReconciliationExecution(customResourceID); | |||
| submitReconciliationExecution(executionScope.getTriggeringEvent()); | |||
There was a problem hiding this comment.
Same here, cannot we avoid using the event?
There was a problem hiding this comment.
Well, the event provide more information about what happened. We could not have that information but the recording would be less interesting I think. Look at the MicrometerMetrics implementation to see how it's used.
|
Replaced by this PR: #622 |
Fixes #598