Improve minion observer stats to capture more granular stages of minion task execution #15118
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15118 +/- ##
============================================
+ Coverage 61.75% 63.45% +1.70%
- Complexity 207 1480 +1273
============================================
Files 2436 2748 +312
Lines 133233 154557 +21324
Branches 20636 23831 +3195
============================================
+ Hits 82274 98079 +15805
- Misses 44911 49079 +4168
- Partials 6048 7399 +1351
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| String logMessage = "Caught exception while reading data."; | ||
| observer.accept(new MinionTaskBaseObserverStats.StatusEntry.Builder() | ||
| .withLevel(MinionTaskBaseObserverStats.StatusEntry.LogLevel.ERROR) | ||
| .withStatus(logMessage + " Reason : " + e.getMessage()) |
There was a problem hiding this comment.
nit: The space between Reason and : could be avoided. Reason: the error reason.
| } | ||
| Map<String, MinionTaskBaseObserverStats> progressStatsMap = new HashMap<>(); | ||
| MinionEventObserver observer = MinionEventObservers.getInstance().getMinionEventObserver(subtaskName); | ||
| MinionTaskBaseObserverStats progressStats = observer.getProgressStats(); |
There was a problem hiding this comment.
Won't this produce an NPE if observer is null?
There was a problem hiding this comment.
Yes it will, my bad. Will fix it, thanks for catching it!
| } | ||
| String incomingStage = statusEntry.getStage(); | ||
| if (_taskProgressStats.getCurrentStage() == null) { | ||
| _taskProgressStats.setCurrentStage(incomingStage != null ? incomingStage : MinionTaskState.UNKNOWN.name()); |
There was a problem hiding this comment.
Add comment on conditions when the incoming stage would be null.
|
|
||
| public void start() { | ||
| _startTimeMs = System.currentTimeMillis(); | ||
| _resumeTimeMs = _startTimeMs; |
There was a problem hiding this comment.
What's the real use of the field _resumeTimeMs? Do we expect the same stage to started more than once?
There was a problem hiding this comment.
Yes that's what I was thinking when introducing it. I wanted to keep this flexibility in case we adopt this where a set of stages is ran in a loop, in which case the cumulative stage time will be tracked.
Description
This PR
MinionProgressObserverNewly added stats
_currentStage: while the_currentStateis limited to only holdMinionTaskState, we need more granular idea about which stage the minion task is currently in. This property has hold any task specific stage to give better tracking._endTimestamp_stageTimes: with the new_currentStageproperty, this property stores a map of stage and its start and total time spent. This stat can help user to identify which stage is taking more time and fine tune the task configs accordingly.These new stats are leveraged to breakdown the
SegmentProcessorFrameworkobservability further into map, reduce and segment generate stage.Endpoint to fetch the task progress stats
Path :
/tasks/subtask/progressStatsMethod :
GETQuery params :
subtaskNameSample Response: