Twentyfive #31
Twentyfive #31
Conversation
|
This isn't done yet. |
|
But it could use some careful review around the new Flush() and Done() code |
Codecov Report
@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 40.56% 41.95% +1.38%
==========================================
Files 9 9
Lines 737 827 +90
==========================================
+ Hits 299 347 +48
- Misses 430 465 +35
- Partials 8 15 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| log.parent.span.activeDependents = make(map[int32]*Log) | ||
| } | ||
| log.parent.span.activeDependents[log.span.spanNumber] = log | ||
| return len(log.parent.span.activeDependents) == 1 |
There was a problem hiding this comment.
Why are we strictly checking if there is only one activeDependent? There is no use case where the parent can have many children?
There was a problem hiding this comment.
If there is exactly one active dependent, then before this one was added, there were zero. If we go from zero to one then the parent has activity and hasActivity() needs to be called.
| postCount := log.recursiveDone(true) | ||
| if postCount > 1 && explicit { |
There was a problem hiding this comment.
Wouldn't we always hit this condition due to line 278? I believe I figured it out if I see the publicly exposed Flush() method, which makes sure that recursiveDone is called with false parameter.
logger.go
Outdated
| if requestStillBoring != 0 { | ||
| l.request.base.Boring(false) | ||
| l.request.span.base.Boring(false) | ||
| } | ||
| l.enableFlushTimer() | ||
| l.hasActivity(true) |
There was a problem hiding this comment.
Maybe the condition in line 426 should be changed to if requestStillBoring == 0 {. This is because 0 value indicates that the request has become non-boring (at least that is what I understand).
|
Thanks for the review. This Done/Flush stuff is pretty complicated. |
|
I think I've figured out why the tests are flaky on windows: the clock resolution may be too course to guarantee non-zero durations. |
No description provided.