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

feat(pino): bump pino dependency #188

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jetersen
Copy link

@jetersen jetersen commented Jan 19, 2022

also fixes old package-lock.json and security issues in dev dependencies

Please correct me if I am using semantic commits wrong 😅 I expect this is a feature to upgrade to a newer major version of pino

Made it a draft to use smaller commits and prepare it slowly but keep it ready for CI and reviews.

@jetersen jetersen marked this pull request as draft January 19, 2022 20:27
@jetersen
Copy link
Author

@gr2m I believe we need to reduce scope of what this package is doing.

in v7 it seems like if you want to log to sentry and pino-pretty the ideal way is multiple transports:

const pino = require('pino')
const transport = pino.transport({
  targets: [
    { target: './my-transport.js' },
    { target: 'pino-pretty' },
  ]
})
pino(transport)

right now this package is trying to both log pretty, json and send to sentry 😅

@gr2m
Copy link
Contributor

gr2m commented Jan 21, 2022

I'm open to do multiple transforms instead if that better aligns with how pino works, thank you for digging into it!

also fixes old package-lock.json and security issues in dev dependencies
@jetersen
Copy link
Author

jetersen commented Apr 16, 2022

@gr2m I think I managed to adapt the code to a transformer based on pino-pretty, without needing to split into multiple transformers.

Now I just need to adapt the tests 😅

@jetersen
Copy link
Author

jetersen commented Apr 16, 2022

example.js is definitely working:

[1650146352217] INFO (23463 on DK-JEP-ROCKET): hello future
[1650146352218] ERROR (23463 on DK-JEP-ROCKET): Caught error
    err: {
      "type": "Error",
      "message": "Caught error",
      "stack":
          Error: Caught error
              at Object.<anonymous> (/home/joseph/git/code/probot-pino/example.js:13:15)
              at Module._compile (node:internal/modules/cjs/loader:1103:14)
              at Object.Module._extensions..js (node:internal/modules/cjs/loader:1157:10)
              at Module.load (node:internal/modules/cjs/loader:981:32)
              at Function.Module._load (node:internal/modules/cjs/loader:822:12)
              at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
              at node:internal/main/run_main_module:17:47
      "status": 500,
      "event": {
        "event": "pull_request",
        "id": "123",
        "payload": {
          "installation": {
            "id": "456"
          }
        }
      },
      "headers": {
        "x-github-request-id": "789"
      },
      "request": {
        "headers": {
          "accept": "application/vnd.github.v3+json",
          "authorization": "[Filtered]",
          "user-agent": "probot/10.0.0"
        },
        "method": "GET",
        "url": "https://api.github.com/repos/octocat/hello-world/"
      }
    }
[1650146352218] FATAL (23463 on DK-JEP-ROCKET): Oh no!
    err: {
      "type": "Error",
      "message": "Oh no!",
      "stack":
          Error: Oh no!
              at Object.<anonymous> (/home/joseph/git/code/probot-pino/example.js:39:12)
              at Module._compile (node:internal/modules/cjs/loader:1103:14)
              at Object.Module._extensions..js (node:internal/modules/cjs/loader:1157:10)
              at Module.load (node:internal/modules/cjs/loader:981:32)
              at Function.Module._load (node:internal/modules/cjs/loader:822:12)
              at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
              at node:internal/main/run_main_module:17:47

@jetersen
Copy link
Author

I am not sure how to adapt the tests. Any guidance or help would be appreciated @gr2m

@jetersen jetersen marked this pull request as ready for review April 16, 2022 22:15
@jetersen
Copy link
Author

jetersen commented Jul 22, 2022

@gr2m potentially we could remove this from probot entirely?

This is only for those using sentry and they could hook it up themself inside their applications?
Since with pino they could just add the multi module for sentry, no?

https://www.npmjs.com/package/pino-sentry

npm
@sentry/node transport for pino logger. Latest version: 0.13.0, last published: 18 days ago. Start using pino-sentry in your project by running `npm i pino-sentry`. There are 10 other projects in the npm registry using pino-sentry.

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