Refine the doubling logic for web #418#423
Conversation
duyhuynhdev
force-pushed
the
refine-the-doubling-logic-for-web-#418
branch
2 times, most recently
from
89e0b9a to
4769488
Compare
There was a problem hiding this comment.
Thanks @duyhuynhdev
This is a much less destructive change than the previous PR, so we should be able to get this merged in once we iron out everything. The previous PR still does have other benefits, especially for web, but is just unworkable for cron without a clearer way to know when tasks end.
One more thing that is missing here that was in the previous PR is the changes to store event count as well (and fixes for import). I think that would be useful to have here.
|
|
||
| /** | ||
| * Parse log entry to sample | ||
| * | ||
| * @param \ExcimerLogEntry $entry | ||
| * @return array | ||
| */ | ||
| protected function from_log_entry_to_sample($entry) { | ||
| return [ | ||
| "eventcount" => $entry->getEventCount(), | ||
| "trace" => $entry->getTrace(), | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * Parse log to samples | ||
| * | ||
| * @param \ExcimerLog $log | ||
| * @return array | ||
| */ | ||
| protected function from_log_to_samples($log) { | ||
| $samples = []; | ||
| foreach ($log as $entry) { | ||
| $samples[] = $this->from_log_entry_to_sample($entry); | ||
| } | ||
| return $samples; | ||
| } |
There was a problem hiding this comment.
These work, but I can't help but think there might be a simpler way to do this without so many changes.
There was a problem hiding this comment.
I also have no idea how to make it simpler ^^. Let me think about after solving all main issues
duyhuynhdev
force-pushed
the
refine-the-doubling-logic-for-web-#418
branch
2 times, most recently
from
b854424 to
bca2036
Compare
|
Hi Ben, I have updated the code. Could you review it again when you back. Thanks |
bwalkerl
left a comment
bwalkerl
left a comment
There was a problem hiding this comment.
This is looking close, the main issue now is just the discrepancy with the duration in the graph, possibly due to a bug in merging or other code needing to be updated.
I've a few new comments alongside the previous ones.
duyhuynhdev
force-pushed
the
refine-the-doubling-logic-for-web-#418
branch
from
bca2036 to
28cf75d
Compare
bwalkerl
left a comment
bwalkerl
left a comment
There was a problem hiding this comment.
Thanks @duyhuynhdev
The sample set abstract class looks great and we've solved the biggest issue, just a few more things to tidy up.
duyhuynhdev
force-pushed
the
refine-the-doubling-logic-for-web-#418
branch
2 times, most recently
from
88db363 to
596aff1
Compare
There was a problem hiding this comment.
This looks to be working as intended from my testing.
There can still be edge cases such as two samples with a large number of events next to each other, but this should be much better than blinding dropping half.
Happy for this to be merged, as long as we are OK with the terminology:
- One thing that can be discussed further is the terminology differences between sample counts and event counts. We have replaced sample counts with event counts, and stored the actual sample counts separately. This does look messy. Would we be better off keeping eventcounts as numsamples and add a new numlogentires column instead?
There was a problem hiding this comment.
@duyhuynhdev After thinking about it some more I think we have the update the terminology for consistency.
- Leave numsamples alone, and change the UI back to 'samples' instead of 'events'. This will be the eventcount, as it was before this update. We don't add numevents.
- Instead of storing the number of log entries as numsamples, store it as numlogentires or numentries
- There was a fallback method when numevents doesn't exist that can be removed.
This would keep consistency with the terminology in the UI and tables, and should reduce confusion.
duyhuynhdev
force-pushed
the
refine-the-doubling-logic-for-web-#418
branch
2 times, most recently
from
8d45cdb to
ef53659
Compare
duyhuynhdev
force-pushed
the
refine-the-doubling-logic-for-web-#418
branch
from
ef53659 to
43d9bdf
Compare
There was a problem hiding this comment.
Thanks @duyhuynhdev this is looking good to me now, matches the spec, and the confusion has been removed so happy for it to be merged.
The only thing left I can see that might be a concern is the size of the samplepool when we reach high filterrates, but that can be mitigated by settings so I'm not sure how much of a concern it is.
duyhuynhdev
force-pushed
the
refine-the-doubling-logic-for-web-#418
branch
from
43d9bdf to
c768862
Compare
duyhuynhdev
force-pushed
the
refine-the-doubling-logic-for-web-#418
branch
from
c768862 to
d3385c6
Compare
3 participants
#418
Add new doubling logic for web processor only
Update related classes and tests