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

Q: Any plan to migrato to v7+ transport? #33

Closed
rodmoreno opened this issue Oct 13, 2021 · 13 comments
Closed

Q: Any plan to migrato to v7+ transport? #33

rodmoreno opened this issue Oct 13, 2021 · 13 comments

Comments

@rodmoreno
Copy link

Recently I read about new v7+ transports and I would like to know if you have any plan to support this new architecture?

@glensc
Copy link
Collaborator

glensc commented Oct 20, 2021

@rodmoreno why would you assume there is a permanent plain to stick old legacy version that you even ask such a question?

aside, I planned to submit a PR once #34 gets merged and I have time to play with this. the API seems very simple.

@glensc
Copy link
Collaborator

glensc commented Oct 21, 2021

Does pino 7.x support 6.x transports? or they are mutually exclusive?

@glensc
Copy link
Collaborator

glensc commented Oct 21, 2021

The pino-abstract-transport for writing transport does not have ts typing:

@glensc
Copy link
Collaborator

glensc commented Mar 13, 2022

Does pino 7.x support 6.x transports? or they are mutually exclusive?

I've done local testing, and seems 0.11 can work with pino 7.x.

how to outline this better in this project, in readme? as pino not even included in dependencies, i.e to be installed via peerDependencies?

@rasmuslp
Copy link

how to outline this better in this project, in readme? as pino not even included in dependencies, i.e to be installed via peerDependencies?

If you use peerDependencies, not that there is different behaviour depending on the version of NPM. I came across that when upgrading my eslint config to the latest airbnb.

With that said, peerDependencies sounds like what you would want to use for 'plugins', as they refer to it in the package.json documentation here: https://docs.npmjs.com/cli/v8/configuring-npm/package-json#peerdependencies

For some inspiration, you can see how they communicate dealing with peerDependencies here: https://www.npmjs.com/package/eslint-config-airbnb
And how I did here: https://github.com/rasmuslp/eslint-config-rasmuslp

@meotimdihia
Copy link

@glensc Does it really work with Pino 7?
I am getting this error by a simple example:

import pino from "pino"

async function main() {
  const transport = pino.transport({
    targets: [
      {
        target: "pino-sentry",
        options: {
          dsn: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
        },
        level: "error"
      }
    ],
  })
  const logger = pino(
    transport
  )
  logger.error('test')
}
main()
  .catch(async e => {
    console.error(e)
  })
node dist/src/examples/test3.js
events.js:377
      throw er; // Unhandled 'error' event
      ^

TypeError: fn is not a function
    at C:\sources\a-cli\node_modules\pino\lib\worker.js:26:26
    at async Promise.all (index 0)
    at async module.exports (C:\sources\a-cli\node_modules\pino\lib\worker.js:13:13)        
    at async start (C:\sources\a-cli\node_modules\thread-stream\lib\worker.js:57:17)        
Emitted 'error' event on process instance at:
    at emitUnhandledRejectionOrErr (internal/event_target.js:579:11)
    at MessagePort.[nodejs.internal.kHybridDispatch] (internal/event_target.js:403:9)       
    at MessagePort.exports.emitMessage (internal/per_context/messageport.js:18:26)
    ```

@glensc
Copy link
Collaborator

glensc commented Apr 23, 2022

@meotimdihia I'm not using v7 api, then it works

the code is part of other framework, so some bits may be off

import { createWriteStream as createSentryStream } from "pino-sentry/dist/transport";
import { Level, Logger as BaseLogger, pino } from "pino";

const streams = [];
const sentryStream =  createSentryStream(sentryOptions);
streams.push({
        stream: sentryStream as any,
        level: (options.levels?.sentry || "error") as Level,
        name: "sentry",
});

const logger = pino(
  {
    level,
  },
  pino.multistream(streams),
);

See also this discussion:

@meotimdihia
Copy link

meotimdihia commented Apr 23, 2022

The same error is happening with the code: (node.js 14)

import pino from "pino"
import { createWriteStream as createSentryStream } from "pino-sentry/dist/transport"

const streams = []
const sentryStream = createSentryStream({
  dsn: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx,
})
streams.push({
  stream: sentryStream,
})

const logger = pino(
  {
    level: 'debug'
  },
  pino.multistream(streams)
)

 logger.error('test')

Referrer: pinojs/pino#1383 (comment)

@glensc
Copy link
Collaborator

glensc commented Apr 24, 2022

@meotimdihia the exact code (after fixing the unterminated dsn string) produces no output for me, so no error.

your previous code I get "the worker has exited" error

node_modules/thread-stream/index.js:281
      throw new Error('the worker has exited')
      ^

Error: the worker has exited
    at ThreadStream.flushSync (node_modules/thread-stream/index.js:281:13)
    at process.onExit (node_modules/pino/lib/transport.js:65:12)
    at process.emit (events.js:412:35)
    at process.emit (domain.js:475:12)
    at process.emit.sharedData.processEmitHook.installedValue [as emit] (node_modules/@cspotcode/source-map-support/source-map-support.js:613:40)
    at process.exit (internal/process/per_thread.js:179:15)
    at process.emit.sharedData.processEmitHook.installedValue [as emit] (node_modules/@cspotcode/source-map-support/source-map-support.js:618:17)
    at process._fatalException (internal/process/execution.js:167:25)

node v14.19.1, pino@7.8.0, pino-sentry@0.11.0, pino-multi-stream@6.0.0

@glensc
Copy link
Collaborator

glensc commented Apr 24, 2022

perhaps the node version is relevant?

@glensc
Copy link
Collaborator

glensc commented Jun 10, 2022

@aandrewww perhaps close this one (and lock), as it's mixed content and therefore confusing for the reader. it started as a question, but then some user hijacked the issue with their bug report instead of creating their own issue and then disappeared as well.

@segevfiner
Copy link
Contributor

We should be careful here. This might interact even worse with propagating the Sentry scope.

@aandrewww
Copy link
Owner

@glensc @segevfiner agree with you

close

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 a pull request may close this issue.

6 participants