[DevTools] PR3.4 Logs #2914
[DevTools] PR3.4 Logs #2914
Conversation
|
megha-narayanan
force-pushed
the
pr-3.4-logs
branch
from
3dce63b to
77c1b19
Compare
svidgen
left a comment
svidgen
left a comment
There was a problem hiding this comment.
Ideally, would like to ensure magic numbers are constants with notes indicating why the numbers where chosen. If the reason is, "subjectively tuned" or similar, just note that so we know it's not based on any firm guidance or hard measurements.
More importantly, unless I missed it, let's ensure code that runs on intervals accounts for overlaps. This is especially important for async code that won't naturally force each subsequent interval event to wait for the prior call to finish.
| let nextToken: string | undefined = undefined; | ||
| let currentLogStreamName = logStreamName; | ||
| let lastStreamCheckTime = Date.now(); | ||
| const STREAM_CHECK_INTERVAL = 30000; // Check for new streams every 30 seconds |
There was a problem hiding this comment.
Adding constants and explanation/documentation for each of these.
| let lastStreamCheckTime = Date.now(); | ||
| const STREAM_CHECK_INTERVAL = 30000; // Check for new streams every 30 seconds | ||
| let consecutiveEmptyPolls = 0; | ||
| let currentInterval = 2000; // Start with 2 seconds |
|
|
||
| // Reset consecutive empty polls and speed up polling if needed | ||
| consecutiveEmptyPolls = 0; | ||
| if (currentInterval > 2000) { |
| if (currentInterval > 2000) { | ||
| // Speed up polling if we're getting logs | ||
| clearInterval(pollingInterval); | ||
| currentInterval = 2000; |
| if (consecutiveEmptyPolls > 3 && currentInterval < 10000) { | ||
| // Slow down polling after 3 empty polls | ||
| clearInterval(pollingInterval); | ||
| currentInterval = Math.min(currentInterval * 1.5, 10000); | ||
| pollingInterval = setInterval(fetchLogs, currentInterval); | ||
| this.activeLogPollers.set(resourceId, pollingInterval); |
There was a problem hiding this comment.
Looks like some values that should be constants. Ideally annotated with why those constants were chosen. Whether and/or how they should be tuned.
Also, looks like some repeated logic for killing and re-registering the interval at a new level. Probably better as shared logic to avoid any drift in how the timers are managed.
There was a problem hiding this comment.
yep, doing that. and extracting that into a helper function for shared logic!
| let currentInterval = 2000; // Start with 2 seconds | ||
|
|
||
| const fetchLogs = () => { | ||
| void (async () => { |
There was a problem hiding this comment.
Why is this wrapped in void (async () => {}) instead of const fetchLogs = async () => {}?
There was a problem hiding this comment.
I think I did this for lint at some point? anyway fixing that.
| let consecutiveEmptyPolls = 0; | ||
| let currentInterval = 2000; // Start with 2 seconds | ||
|
|
||
| const fetchLogs = () => { |
There was a problem hiding this comment.
Maybe I'm missing it, but this seems to optimistically assume any previous fetchLogs request has finished. But, that won't always be the case.
What happens if fetchLogs requests start to overlap? (Due to network congestion, service degradations, etc.)
There was a problem hiding this comment.
Good catch. I think I neglected this because of the delay between requests, but I added a check for that now.
megha-narayanan
force-pushed
the
pr-3.4-logs
branch
from
77c1b19 to
bdba176
Compare
2 participants
Implements various parts of log steaming functionality
Note frontend tests for components do exist but are not included in this PR due to dependencies which require PR3.1 being merged.