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

Add Pino for logging to Datadog #421

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Add Pino for logging to Datadog #421

merged 1 commit into from
Aug 23, 2022

Conversation

simonauner
Copy link
Contributor

@simonauner simonauner commented Aug 23, 2022

So, my draft PR got a bit messy with all the commits and re-bases #414, starting from a clean slate here.

Describe your changes

Added Pino set up with a log forwarder/transport that sends logs to Datadog.

Had to add topLevelAwait allow Pino's init function to work.

Removed server-preload.js which doesn't seem to work anymore. In it we monkey-patched console.log and added Datadogs tracing lib dd-trace, but it wasn't applied anyway, so might as well remove it to remove confusion about how we log.

Justify why they are needed

We already have Datadog set up as an integration in Vercel (https://vercel.com/hedvig/racoon/settings/integrations) which already forwards all Next.js logs to Datadog. This does not cover console.logs though - all we get is request logs:

Screenshot 2022-08-23 at 13 05 55

With Pino in place we have a way to "debug" server code in prod/staging.

Jira issue(s): GRW-1388

Checklist before requesting a review

  • I have performed a self-review of my code

- Enable topLevelAwait in Webpack, needed for Pino logger
- Remove server-preload.js, Vercel doesn't run it anyway
@vercel
Copy link

vercel bot commented Aug 23, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
racoon ✅ Ready (Inspect) Visit Preview Aug 23, 2022 at 11:10AM (UTC)
1 Ignored Deployment
Name Status Preview Updated
racoon-store ⬜️ Ignored (Inspect) Aug 23, 2022 at 11:10AM (UTC)

@@ -37,7 +36,8 @@
"marked": "4.0.18",
"next": "12.2.2",
"next-i18next": "^11.0.0",
"pino": "^7.10.0",
"pino-datadog": "https://github.com/simonauner/pino-datadog#rename-hostname-to-host",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently running a customized version of pino-datadog because of ovhemert/pino-datadog#89

@@ -128,7 +128,7 @@ self.addEventListener('fetch', function (event) {
// At this point, any exception indicates an issue with the original request/response.
console.error(
`\
[MSW] Caught an exception from the "%s %s" request (%s). This is probably not a problem with Mock Service Worker. There is likely an additional logging output above.`,
[MSW] Caught an exception from the "%s %s" request (%s). This is probably not a problem with Mock Service Worker. There is likely an additional logging output above.`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accidentally opened this file and saved it, and prettier performed its magic 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i think you can regenerate with cd apps/store/ && npx msw init ./public && cd -

version: process.env.NEXT_PUBLIC_DATADOG_VERSION,
env: process.env.NEXT_PUBLIC_DATADOG_ENV || 'dev',
env: process.env.NEXT_PUBLIC_DATADOG_ENV || 'local',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dev is sometimes referred to as our staging environment, local cannot be misinterpreted as anything other than "running on my machine"?

// URL will be racoon-onboarding.vercel.app when deployed on Vercel, but empty string when running locally.
// NEXT_PUBLIC_DATADOG_SERVICE_NAME will be racoon-onboarding, so this is just a way to set a meaningful
// service name when running locally.
service: process.env.VERCEL_URL || `${process.env.NEXT_PUBLIC_DATADOG_SERVICE_NAME}.local`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's see what process.env.VERCEL_URL translates to when deployed to staging and prod. It might be that it's the machine-name, not the URL, and then this will have to be changed to something else.

@simonauner simonauner marked this pull request as ready for review August 23, 2022 11:15
@simonauner simonauner requested a review from a team as a code owner August 23, 2022 11:15
Copy link
Contributor

@robinandeer robinandeer left a comment

Choose a reason for hiding this comment

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

No major comments from me

@@ -128,7 +128,7 @@ self.addEventListener('fetch', function (event) {
// At this point, any exception indicates an issue with the original request/response.
console.error(
`\
[MSW] Caught an exception from the "%s %s" request (%s). This is probably not a problem with Mock Service Worker. There is likely an additional logging output above.`,
[MSW] Caught an exception from the "%s %s" request (%s). This is probably not a problem with Mock Service Worker. There is likely an additional logging output above.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i think you can regenerate with cd apps/store/ && npx msw init ./public && cd -

@simonauner
Copy link
Contributor Author

Ok, so apparently there is a new Pino version out which doesn't require pino-multi-stream.... Will work on that instead: #422

@simonauner simonauner merged commit 6bf9280 into main Aug 23, 2022
@simonauner simonauner deleted the GRW-1388/feat/logging2 branch August 23, 2022 13:12
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