Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[next] Update error logging #9129

Merged
merged 2 commits into from
Nov 29, 2023
Merged

[next] Update error logging #9129

merged 2 commits into from
Nov 29, 2023

Conversation

FredKSchott
Copy link
Member

@FredKSchott FredKSchott commented Nov 18, 2023

Changes

  • New, more concise error log formatting
  • Less extremely concise than what I'd originally pitched in Logging refresh #8833, more closely matches the current formatting of an error.
  • After discussion with @Princesseuh and other maintainers, agreed to remove: File ID (already included in stacktrace), codeframe (often poorly formatted), full stack trace (now trimmed to just user code, by default).
  • Updated one hint to be more concise, that @Princesseuh and I had already audited together in Logging refresh #8833

Result

This PR

Screen Shot 2023-11-18 at 12 38 06 AM

Previously

Screen Shot 2023-11-18 at 12 39 56 AM

^ didn't fit in my terminal! Once I dragged to expand and retake the screenshot:

Screen Shot 2023-11-18 at 12 40 09 AM

Testing

N/A

Docs

N/A

Copy link

changeset-bot bot commented Nov 18, 2023

🦋 Changeset detected

Latest commit: a3f2214

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Nov 18, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

'astro': minor
---

Update error log formatting
Copy link
Contributor

Choose a reason for hiding this comment

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

We like to make these changesets a paragraph or two, like what would go in a blog post.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

In favor of this change! Glad we talked through it, thanks for scoping it back a bit.

@@ -20,7 +20,7 @@ export async function throwAndExit(cmd: string, err: unknown) {

const errorWithMetadata = collectErrorMetadata(createSafeError(err));
telemetryPromise = telemetry.record(eventError({ cmd, err: errorWithMetadata, isFatal: true }));
errorMessage = formatErrorMessage(errorWithMetadata);
errorMessage = formatErrorMessage(errorWithMetadata, true);
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking, but it'd be nice to move to an options object like { verbose: true } or { showFullStacktrace: true } to avoid an unlabelled boolean argument.

@matthewp matthewp merged commit 8bfc205 into next Nov 29, 2023
13 checks passed
@matthewp matthewp deleted the logging-rewrite-3 branch November 29, 2023 17:43
@astrobot-houston astrobot-houston mentioned this pull request Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants