Skip to content

Do not stringify log messages in the browser console#40265

Merged
gzdunek merged 7 commits intomasterfrom
gzdunek/better-logging
Apr 9, 2024
Merged

Do not stringify log messages in the browser console#40265
gzdunek merged 7 commits intomasterfrom
gzdunek/better-logging

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Apr 5, 2024

Currently, all log messages from Connect are stringified with JSON.stringify to allow logging complex values like objects or arrays (instead of printing [object Object]). This happens for both file loggers and dev loggers in consoles.

The only downside of this is that reading long log messages (like unified resources response) is quite hard.
To improve readability, we can stop stringifying the values for the browser. Instead, we can use console.* functions to present objects or arrays in a nice, expandable way.

Before:
image

After:
image

Important

This only affects dev logs in the browser, not file or dev logs in a non-browser console (main process).

I also fixed a bug introduced in #39229, where pinnedOnly property started to be considered "sensitive". Instead of making partial matching on the key names, we can check the full name.

@gzdunek gzdunek added teleport-connect Issues related to Teleport Connect. no-changelog Indicates that a PR does not require a changelog entry developer-experience Addressing these issues will improve the experience of developers working on Teleport backport/branch/v15 labels Apr 5, 2024
Comment thread web/packages/teleterm/src/services/tshd/interceptors.ts Outdated
@gzdunek gzdunek marked this pull request as ready for review April 5, 2024 15:22
@gzdunek gzdunek requested review from avatus and ravicious April 5, 2024 15:22
@github-actions github-actions Bot requested review from ibeckermayer and kimlisa April 5, 2024 15:22
Comment thread web/packages/teleterm/src/services/tshd/interceptors.ts Outdated
Comment thread web/packages/teleterm/src/services/logger/loggerService.ts
Comment thread web/packages/teleterm/src/services/logger/loggerService.ts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The only thing that worries me is that there's no way to expand all collapsed objects at once. This means that:

  1. You won't see the output as it comes, e.g. when the app continuously receives some kind of messages from tshd.
  2. You won't be able to filter by parts of the output that have been collapsed.

The workaround for both of those things would be to simply look at the log file instead. Also, you can use Opt + click to recursively expand any given object, but that still works on only a single object.

I think we should be able to live with that, what do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Correct, but if the response is more complex, looking for a particular part of it is hard when the message is unformatted.
  2. I think I've never had a need to look for something in more than one response, so expanding it wouldn't be too much work.

Overall, your points are valid, but the pros still outweigh the cons to me.
You also provided a good workaround, so I think we don't loose anything here.

@gzdunek gzdunek added this pull request to the merge queue Apr 9, 2024
Merged via the queue into master with commit 9ba9f5d Apr 9, 2024
@gzdunek gzdunek deleted the gzdunek/better-logging branch April 9, 2024 10:33
@public-teleport-github-review-bot
Copy link
Copy Markdown

@gzdunek See the table below for backport results.

Branch Result
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

developer-experience Addressing these issues will improve the experience of developers working on Teleport no-changelog Indicates that a PR does not require a changelog entry size/sm teleport-connect Issues related to Teleport Connect. ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants