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

Proposal: Using references to log messages in logging hooks #1705

Open
cromedome opened this issue Dec 18, 2023 · 2 comments
Open

Proposal: Using references to log messages in logging hooks #1705

cromedome opened this issue Dec 18, 2023 · 2 comments

Comments

@cromedome
Copy link
Contributor

I was making some minor corrections in the logging docs when I happened upon hooks in our logging engine (what a nice surprise!). I noticed they were not documented, and as I tried to use them to make some examples, I got a bit frustrated by them. I'd like to discuss a possible change.

In Dancer2::Core::Role::Logger, we have:

around 'log' => sub {
    my ($orig, $self, @args) = @_;

    $self->execute_hook( 'engine.logger.before', $self, @args );
    $self->$orig( @args );
    $self->execute_hook( 'engine.logger.after', $self, @args );
};

In other places (such as Dancer2::Core::Role::Template), we pass around a reference so we can manipulate content in the hooks before continuing execution. This isn't possible in the logger hook, at least not trivially (unless I am missing something - which is completely likely!). So you'd have to push that functionality down into the logging adapter/logger engine, or create another wrapper method for log().

What are your thoughts about making @args a reference instead? This would allow developers to add additional information to log messages (such as a request ID or other metadata) without repeating code.

On the other hand, you can make a solid argument for not wanting to alter log messages. And maybe that was the original intent. I wasn't here for that. If that is the case, I can close this and you all can disregard.

Dancers, what are your thoughts here? Am I simply missing the point of what you're supposed to do with this hook?

@GeekRuthie
Copy link
Contributor

I'd suggest making the change, and then giving it a try; make sure it passes all the tests, etc. But I'm all for being able to manipulate that data in a hook. This shouldn't be too large a change, I wouldn't think.

@SysPete
Copy link
Member

SysPete commented Dec 19, 2023

Although it is unlikely that this hook is heavily used, especially since it is undocumented, we still risk breaking live code if this change is made. I've know I've used undocumented hooks in the past, but I can't remember which ones, and this may well be one of them, so I'm not keen on this change.

When adding extra data to logs, my preference is to do that using Log4perl, though still leaving the original message intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants