Skip to content

Show stack for redactable error in log#3004

Merged
koesie10 merged 4 commits intomainfrom
koesie10/redactable-error-stack
Oct 23, 2023
Merged

Show stack for redactable error in log#3004
koesie10 merged 4 commits intomainfrom
koesie10/redactable-error-stack

Conversation

@koesie10
Copy link
Copy Markdown
Member

When calling for example showAndLogExceptionWithTelemetry, the stack trace would be sent to Application Insights, but there was no way to see the stack trace from within VS Code. This will add the stack trace to the log by returning it from fullMessageWithStack and using it in the appropriate places.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

When calling for example `showAndLogExceptionWithTelemetry`, the stack
trace would be sent to Application Insights, but there was no way to
see the stack trace from within VS Code. This will add the stack trace
to the log by returning it from `fullMessageWithStack` and using it in
the appropriate places.
const error = result.message
? redactableError`${result.message}`
: redactableError`Failed to run query`;
void extLogger.log(error.fullMessage);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was removed because it results in duplicate logs: the showAndLogExceptionWithTelemetry will also log it to the extLogger.

@koesie10 koesie10 marked this pull request as ready for review October 20, 2023 14:50
@koesie10 koesie10 requested a review from a team as a code owner October 20, 2023 14:50
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Thanks for tidying that up!

return this.fullMessage;
}

return `${this.fullMessage}\n${this.stack}`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we use os.EOL rather than \n?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unfortunately we can't use that because the errors.ts module is also imported from the webview which doesn't have access to os.EOL. I'm not sure whether \n works in VS Code's log output, but I don't think it's worth the complexity of trying to determine whether we're in the webview and then deciding to use either os.EOL or \n.

This reverts commit b33b5bb.

The errors module is also imported in the webview, so we can't actually
use it.
@koesie10 koesie10 merged commit 41aeb47 into main Oct 23, 2023
@koesie10 koesie10 deleted the koesie10/redactable-error-stack branch October 23, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants