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: Add support for multiple pipelines & using pipelines with static targets #1664

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Starkrights
Copy link

Hi all!

This PR will add support for the usage of regular transports alongside pipelines, as well as the usage of multiple pipelines.

Currently it

  • Adds lib/worker-mixed.js which handles the new targets format
    • Creates target streams the same as worker.js for static targets
    • Utilizes lib/worker-pipeline.js to create the pipeline streams.
  • Modifies lib/transport.js to properly parse the new API.

RFC: @mcollina mentioned in #1302 that he was interested in a new mixed-worker.js file, but I think it might be better off just being added to lib/worker.js. My reasoning being that it's actually not that substantial of a change, and it probably leads to the least overall API change.

Currently, I just have worker-mixed.js shoehorned in where worker.js was previously in lib/transport.js as proof-of-concept, but it shows my point pretty succinctly. I don't immediately see a case where it's useful to have a worker.js that can't handle the pipeline targets, unless a different API is desired.

Suggestions and/or critiques on the topic would be appreciated.

API Change

Allow pipelines to be provided in the targets: [{},{},(...)] array in pino({}) or pino.transport({}) options objects.

Object structure example
opts = {
  targets: [
    { target: 'path/to/target1', options: {/*(...)*/} },
    { target: 'path/to/target2', options: {/*(...)*/} },
    { pipeline: true, targets: {/* pipelineable targets, as in a normal pipeline def */} },
  ]
}

The existing pipeline logic in lib/transport.js can be left untouched for backwards compat. The only real break would be if someone was manually creating pipeline properties on their targets for arbitrary usage. Relevant XKCD

Draft stage checklist

  • Finalize location of worker logic (worker.js or new worker-mixed.js?)
  • Add tests for new transport.js code
  • Update option typings

@ouya99
Copy link

ouya99 commented Jun 10, 2023

@Starkrights any update on this? i would like to use pipelines together with transports as in #1302

@Starkrights
Copy link
Author

@ouya99 Heya! Sorry for the delay, I kept forgetting to get around to your pings when I got home.

From what I remember, I'm pretty sure this impl justworks™️. I don't think the code is spic and span, but I remember having the end goal working. I think I have further changes on my local repo, but I'm not sure what all they were (around the time I started this & made the PR, my semester started picking up like crazy & I more/less just dropped it for the time being)

Sometime tonight I'll pull up my pino repo & the project I implemented it in & give you a ping!I don't believe I ever performance tested it, but I do know that I 100% had the concept itself working fine.

@ouya99
Copy link

ouya99 commented Jun 11, 2023

@Starkrights ping :) - thanks heaps for just pushing what you got

@Starkrights
Copy link
Author

@ouya99 I haven't forgotten you, I promise! Just a hectic weekend.

If you've got discord, feel free to ping me there @Starkrights this evening. I get off work late, but I'll set a reminder for myself as well.

@Starkrights
Copy link
Author

Starkrights commented Jun 13, 2023

@ouya99 Looks like I actually did have everything pushed from the last time I touched this. It's not mentioned in the PR, but I think this should be completely backwards compatible with existing pino options structures.

To use multiple pipelines w/ this code: When providing {transport: {targets: [<target>]}} target objects to pino(), just add the property pipeline: true to the target object. Then you can provide a targets: [] field within that top-level target.

Here's a snippet from the project I implemented this feature in for reference.

const date = new Date(Date.now());

const customLevels = {
  verbose: 15
}

const basePath = join(__dirname, 'log');
const fileDate = `${date.getFullYear()}-${date.getMonth()+1}-${date.getDate()}_${date.getHours()}-${date.getMinutes()}-${date.getSeconds()}`


const logger: Logger = pino({
  transport: {
    targets: [{ /* Regular file transport */
          target: 'pino/file',
          options: { level: 'info', levels: customLevels, destination: join(__dirname, `../../logs/${fileDate} info.log`), mkdir: true }
        },
        { /* Regular file transport */
          target: 'pino/file',
          options: { level: 'error', levels: customLevels, destination: join(__dirname, `../../logs/${fileDate} error.log`), mkdir: true }
        },
        { /* Pipeline transport */
          pipeline: true,
          targets: [{ /* Transports to be used in this pipeline */
              target: join(basePath, '<preprocessorTransport>.js'),
              options: {levels: customLevels}
            },
            {
              target: join(basePath, '<finalDestinationTransportA>.js'),
            }
          ]
        },
        { /* Pipeline transport */
          pipeline: true,
          targets: [{ /* Transports to be used in this pipeline */
              target: join(basePath, '<preprocessorTransport>.js'),
              options: {levels: customLevels}
            },
            {
              target: join(basePath, '<finalDestinationTransportB>.js'),
              options: {levels: customLevels}
            },{
              target: 'pino/file'
            }
          ]
        }
      ]
  },
  customLevels,
  level: 'trace'
})

@ouya99
Copy link

ouya99 commented Jun 14, 2023

@Starkrights looking good and tested. working. Thx for sample code! any chance we see this in PR merged soon @mcollina ?

@mcollina
Copy link
Member

The PR is still draft and docs are missing. Code and tests seems ok.

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.

3 participants