Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Limit logs and write logs to file that can be opened by user #263

Merged
merged 2 commits into from Aug 4, 2017

Conversation

dpgraham
Copy link
Contributor

@dpgraham dpgraham commented Aug 3, 2017

  • Limit logs to 1000
    • When logs get above 1000, the app slows down/crashes. Limit the number of rendered logs to 1000
  • Log writing
    • Main thread writes all logs to file
    • Main thread sends the filepath of the logs to the browser window
    • Added 'GetRawLogsButton' to server monitor component
    • When button is clicked, opens the file externally
    • Delete logfile when browser closed
    • If logfile not present, show an alert to indicate that logs can't be accessed

@dpgraham dpgraham requested a review from jlipps August 3, 2017 20:37
Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made a few comments. also i've scanned this a few times and i don't see where you're actually chopping off the log entries in the browser

if (batchedLogs.length) {
win.webContents.send('appium-log-line', batchedLogs);
try {
await fs.writeFile(logFile, batchedLogs.map((log) => `[${log.level}] ${log.msg}`).join('\n'), {'flag': 'a'});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need quotes around flag

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would look better on multiple lines. one line for the map, the other for the write.

@@ -64,6 +64,16 @@
border-color: #a0a0a0 !important;
}

.getRawLogsButton {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we share some of this code with .stopButton?

@dpgraham dpgraham force-pushed the dpgraham-testobject-ui-updates branch 2 times, most recently from 4a61436 to 0ac864a Compare August 3, 2017 23:54
@dpgraham dpgraham force-pushed the dpgraham-log-rendering-limit branch from a560252 to ea6be69 Compare August 3, 2017 23:56
@dpgraham dpgraham changed the base branch from dpgraham-testobject-ui-updates to master August 3, 2017 23:57
@dpgraham
Copy link
Contributor Author

dpgraham commented Aug 3, 2017

Fixed the branch so that you can see the slicing in this commit: f0c997d

@dpgraham
Copy link
Contributor Author

dpgraham commented Aug 4, 2017

Fixes pushed

@@ -37,13 +40,21 @@ export default function serverMonitor (state = initialState, action) {
sessionId: action.sessionUUID,
};
case LOGS_RECEIVED:
// TODO: We should dump logs to a txt file that can be exported
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is no longer a TODO, right?

@@ -37,13 +40,21 @@ export default function serverMonitor (state = initialState, action) {
sessionId: action.sessionUUID,
};
case LOGS_RECEIVED:
// TODO: We should dump logs to a txt file that can be exported
var logLines = state.logLines.concat(action.logs.map((l) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an eslint rule. Apparently you can't declare using let inside of switch statements.

I'll just move the declaration outside of it instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, case statements aren't blocks, so the variables are hoisted to the top of the switch statement, but uninitialized until the case it executed. This isn't so good.

The best solution would be to wrap the case stuff into a block.

case LOGS_RECEIVED: {
  // TODO: We should dump logs to a txt file that can be exported
  let logLines = state.logLines.concat(action.logs.map((l) => {
    // attach a timestamp to each log line here when it comes in
    l.timestamp = moment().format('YYYY-MM-DD hh:mm:ss');
    return l;
  }));

  // Don't log more than MAX_LOG_LINES
  if (logLines.length > MAX_LOG_LINES) {
    logLines = logLines.slice(logLines.length - MAX_LOG_LINES);
  }

  return {
    ...state,
    logLines,
    serverStatus: STATUS_RUNNING
  };
}

* Main thread writes all logs to file
* Main thread sends the filepath of the logs to the browser window
* Added 'GetRawLogsButton' to server monitor component
* When button is clicked, opens the file externally
* Delete logfile when browser closed
* If logfile not present, show an alert to indicate that logs can't be accessed
@dpgraham dpgraham force-pushed the dpgraham-log-rendering-limit branch from 2f7a07c to 0c93577 Compare August 4, 2017 16:40
@dpgraham dpgraham merged commit 68150ef into master Aug 4, 2017
@dpgraham dpgraham deleted the dpgraham-log-rendering-limit branch August 4, 2017 16:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants