Add ParallelStep method to Trace to support logging concurrent work with an explicit duration#348
Conversation
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: richabanker The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/M
| for _, stepOrTrace := range t.traceItems { | ||
| stepOrTrace.rLock() | ||
| stepOrTrace.writeItem(b, formatter, lastStepTime, stepThreshold) | ||
| lastStepTime = stepOrTrace.time() |
There was a problem hiding this comment.
@serathius @dashpole I am wondering how should lastStepTime behave for a parallel step?
If ParallelStep advances lastStepTime normally, any sequential step that follows is measured from when ParallelStep was recorded. But if we consider a case like
t=0 trace starts; background i/o goroutine launched, CPU work begins on main thread
t=50 i/o finishes - ParallelStep("i/o", 50ms) advances lastStepTime to t=50
t=70 CPU finishes - Step("cpu computation") shows 20ms ---> should this be 70ms ?
so, should ParalleStep freeze lastStepTime to support this case ?
There was a problem hiding this comment.
I think we could also just call this AggregatedStep or something, if the only use case is to provide the aggregate of durations spent in various stages of a streamed response. ParallelStep seems to indicate that this could happen in the middle of a trace, which I am not sure is something we want (probably needs bigger discussion, and is also not the use case we are trying to solve in kubernetes/kubernetes#137181)
|
cc @p0lyn0mial |
…ith an explicit duration Signed-off-by: Richa Banker <richabanker@google.com>
richabanker
force-pushed
the
parallel-event-traces
branch
from
a8d1510 to
1eac8c7
Compare
|
/retest |
|
Here is an alternative: kubernetes/kubernetes#138873 |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds
ParallelStepmethod to Trace to support logging concurrent work with an explicit duration.Which issue(s) this PR fixes:
Issue #kubernetes/kubernetes#136002
Special notes for your reviewer:
Release note: