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

fix: Fix logFormatter when no undefined ns, name, or message. #8181

Merged
merged 4 commits into from
May 4, 2023

Conversation

dthyresson
Copy link
Contributor

@dthyresson dthyresson commented May 1, 2023

Fix #8180.

When the log formatter encounters log statements that lack the ns or log name or no message attribute -- say from Fastify or Yoga debug logging, the message includes "undefined".

image

image

Note that when the graphql-server logs, the child logger has a name, but not a ns value.

The PR checks for the undefined cases when formatting the values and also adds a test to ensure that the text "undefined" does not appear in the formatted log content when ns, name or message are not provided.

@dthyresson dthyresson added the release:fix This PR is a fix label May 1, 2023
@dthyresson dthyresson self-assigned this May 1, 2023
@dthyresson
Copy link
Contributor Author

dthyresson commented May 1, 2023

Unable to test with yarn rwfw to show updated screens.

@thedavidprice
Copy link
Contributor

@dthyresson what help do you need to get this one in (if anything)?

Copy link
Contributor Author

I just need to test with rwfw . My unit tests pass. Then a review. I’ve got it in morning

@thedavidprice
Copy link
Contributor

@dthyresson Roger that 👍 Was going to suggest you try testing via canary package if needed. Keep me/us posted.

@dthyresson dthyresson changed the title fix: Fix issue 8180. No undefined ns, name, or message. fix: Fix logFormatter when no undefined ns, name, or message. May 2, 2023
@dthyresson
Copy link
Contributor Author

Tested in GitPod and looks good
image

@dthyresson dthyresson requested a review from Tobbe May 2, 2023 14:10
Copy link
Contributor

@thedavidprice thedavidprice left a comment

Choose a reason for hiding this comment

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

Good for me ✅

Not sure if you were simply looking for another pair of eyes or had specific questions for Tobbe. Deferring to you re: next steps here.

@dthyresson dthyresson enabled auto-merge (squash) May 4, 2023 01:43
@jtoar jtoar disabled auto-merge May 4, 2023 04:38
@jtoar jtoar merged commit 8353290 into redwoodjs:main May 4, 2023
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone May 4, 2023
@jtoar jtoar modified the milestones: next-release, next-release-patch May 4, 2023
@jtoar jtoar modified the milestones: next-release-patch, v5.0.4 May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug?]: Log Formatter includes extraneous "undefined" text labels when missing certain properties
3 participants