Skip to content

Conversation

CSC-swesters
Copy link
Contributor

…PID in front

  • When logging inside the Shell app, include the username, the target host and the pty-term PID number in each message, to see which connection is causing the message. Prior to this, it was impossible to see which terminal connected to which host (since there could be multiple simultaneous terminals), and which terminal was causing potential error messages.

Fixes #4626

…PID in front

- When logging inside the Shell app, include the username, the target host
  and the pty-term PID number in each message, to see which connection is
  causing the message. Prior to this, it was impossible to see which terminal
  connected to which host (since there could be multiple simultaneous
  terminals), and which terminal was causing potential error messages.
Comment on lines 191 to 193
setpid: (newpid) => {
msgPrefix = `[User = ${username}; Host = ${host}; PID = ${newpid}]`;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to do this in a more functional style so we don't have msgPrefix as a variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The challenge is that the PID isn't known when the logger is initialized. The PID will be known once the code gets to this line:

https://github.com/OSC/ondemand/pull/4628/files#diff-e21ee4d7161757c9f1dbb11896bbcbc3c2749663d5f6153873a7e98ae7f20f10L214

 term = pty.spawn(cmd, args, {
//...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One solution is to log the Web socket connection established from user message with a normal console.log(), and initialize the ws.logger once the pty-term PID is known. I'll push an update.

- Do not initialize the ws.logger until the pty-term PID is known.
- Add a few comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Awaiting Review
Development

Successfully merging this pull request may close these issues.

Shell app: improve log traces
3 participants