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

Childlogger Loglevel not respected in the browser #960

Open
KeKs0r opened this issue Feb 5, 2021 · 8 comments
Open

Childlogger Loglevel not respected in the browser #960

KeKs0r opened this issue Feb 5, 2021 · 8 comments

Comments

@KeKs0r
Copy link

KeKs0r commented Feb 5, 2021

Hi,

I am using pino in the browser, since I have some parts of my application that are used in the browser as well as on node.

When using pino in the browser, it seems that different log levels for Child loggers are not working:
Here is my root logger

const logger = pino({
    level: 'info',ser: {
        asObject: false,
        serialize: false,

        transmit: {
            level: 'error',
            send: function (level, logEvent) {
             //... Send error to sentry
            },
        },
    },
})

const childLogger = logger.child({name:'child', level:'debug'})

logger.debug("Root Logger Debug") // not logged
logger.info("Root Info") // logged
childLogger.debug("Child Debug") // Not logged, but I think it should
childLogger.info("Child Info") // logged

The output looks like this
image

It seems the level for the child logger, is just taken as additional property, instead of changing the level of the child logger.

@mcollina
Copy link
Member

mcollina commented Feb 5, 2021

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@jakobrosenberg
Copy link

jakobrosenberg commented May 8, 2021

I had a similar issue which I solved in Chrome. Not sure if it's relevant.

image

@NicoVogel
Copy link
Contributor

I have the same problem in the browser.
Here a short reproducible setup: https://stackblitz.com/edit/typescript-5ka82t?devToolsHeight=33&file=index.ts

@mcollina As this feature will be quite handy in the upcoming future for us, I will look into it.

@GeorgEchterling
Copy link

The fix in #1725 only works partially. Setting the level via child.level = "debug" works, but providing it as an option in root.child() does not:

const root = pino({ level: "info" })
root.info("OK: root info WILL be logged.")
root.debug("OK: root debug will NOT be logged.")

const child1 = root.child({})
child1.level = "debug"
child1.info("OK: child1 info WILL be logged.")
child1.debug("OK: child1 debug WILL be logged.")

const child2 = root.child({}, { level: "debug" })
child2.info("OK: child2 info WILL be logged.")
child2.debug("ERROR: child2 debug will NOT be logged.") // <--

@NicoVogel
Copy link
Contributor

@GeorgEchterling the code for the browser logger takes a while to get into. Don't know if I will manage to work on it in the near future to fix this issue.

@stevel032
Copy link
Contributor

I encountered the same problem of child logger level not effective when passed as an option, but it does work if explicitly set after the creation of such child logger. I looked at the code, while the browser logger code does take a while to get into, but this problem seems to be rather obvious. The option.level variable is simply not used:

function child (bindings, childOptions) {
    if (!bindings) {
      throw new Error('missing bindings for child Pino')
    }
    childOptions = childOptions || {}

    ... quite a bit of code to stuff, but childOptions.level is never used. ...

    ... and then the new child logger inherits the parent level and is immediately returned ...

    // required to actually initialize the logger functions for any given child
    newLogger.level = this.level

    return newLogger
  }

I'm very much new to Pino and it seems that supporting browser side logging isn't the main use case. But in any case, if the experienced folks here think it should be as simple as setting the level before returning the new child logger, I'm happy to submit a simple PR.

@mcollina
Copy link
Member

mcollina commented Jun 9, 2024

Yes! That would be great.

@stevel032
Copy link
Contributor

I just submitted a very simple PR, you guys are encouraged to run the new test case against exiting code and you should be able to see the relatively obvious problem, as the docs (https://github.com/pinojs/pino/blob/main/docs/api.md#child) clearly says this about setting a child logger's level:

It can be set independently of the parent either by ... ... or using the options.level key

mcollina pushed a commit that referenced this issue Jun 11, 2024
… at cr… (#1986)

* Fixing browser side child log issue, child level can now be set at creation time, #960

* Added test case for setting child logger level at creation time (#960)

---------

Co-authored-by: Steven Li <steve98@gmail.com>
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

No branches or pull requests

7 participants
@mcollina @KeKs0r @jakobrosenberg @NicoVogel @stevel032 @GeorgEchterling and others