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.

Sign up for GitHub

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

Jump to bottom

Contextual logging #1404

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Contextual logging #1404

merged 1 commit into from
Sep 11, 2019

Conversation

kraih
Copy link
Member

@kraih kraih commented Sep 9, 2019 •
edited
Loading

This is one possible solution for #1400. The main goals were performance and simplicity. And there would be no backwards compatibility breakage.

@kraih
Copy link
Member Author

kraih commented Sep 9, 2019

Not putting this up for a vote yet (will wait a few days), because some people on IRC have been working on alternative proposals.

@kraih kraih force-pushed the log_context branch 2 times, most recently from a59bf65 to 7392c2f Compare September 9, 2019 20:17
@kraih
Copy link
Member Author

kraih commented Sep 9, 2019

I've also updated Mojolicious default log messages to include the request id when possible. Performance cost seems pretty minimal, less than 2% difference for examples/hello-template.pl.

@kraih
Copy link
Member Author

kraih commented Sep 10, 2019

Since it now looks like nobody else is working on an alternative proposal, i'll just call for a vote @mojolicious/core.

christopherraa
christopherraa approved these changes Sep 10, 2019
View reviewed changes
Copy link
Member

@christopherraa christopherraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I really like that the change is so small (disregarding the change from $self->log to $self->helpers->log various places). Also went through some implementations we have that do things with the logging (custom formatters etc) and this change does not present itself as any kind of inconvenience.

👍

jberger
jberger approved these changes Sep 10, 2019
View reviewed changes
Copy link
Member

@jberger jberger left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might want to revisit structured logging at a major version number, but I'm very happy to have this! +1

@kraih kraih added the vote label Sep 10, 2019
@kraih
Copy link
Member Author

kraih commented Sep 10, 2019

@jberger But if we are already planning to revisit structured logging, shouldn't the context method be designed around that? Like $log->context('foo', 'bar') instead of the current $log->context('[foo] [bar]')?

@jberger
Copy link
Member

jberger commented Sep 10, 2019

For now, I think assuming a string in the context is fine. The way I see it, later any scalar could be used, including a data structure, and we'd have some additional formatter that would handle structured data when given. That's just my thought though

@kraih
Copy link
Member Author

kraih commented Sep 10, 2019

That is true, $log->context(['foo', 'bar']) would work just as well later on.

jhthorsen
jhthorsen reviewed Sep 11, 2019
View reviewed changes
Copy link
Member

@jhthorsen jhthorsen left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments should not be considered blockers for this PR, but I don't want to approve it until I know the reasoning for the design.

😐

lib/Mojolicious/Plugin/DefaultHelpers.pm Show resolved Hide resolved
lib/Mojo/Log.pm Show resolved Hide resolved
marcusramberg
marcusramberg requested changes Sep 11, 2019
View reviewed changes
Copy link
Member

@marcusramberg marcusramberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight typo. Otherwise lgtm and 👍.

lib/Mojo/Log.pm Outdated Show resolved Hide resolved
@kraih kraih mentioned this pull request Sep 11, 2019
Closed
marcusramberg
marcusramberg reviewed Sep 11, 2019
View reviewed changes
Copy link
Member

@marcusramberg marcusramberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

CandyAngel
CandyAngel approved these changes Sep 11, 2019
View reviewed changes
Copy link
Contributor

@CandyAngel CandyAngel left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Looks completely backwards compatible and very useful!

EDIT: Rules are made for followin' (in this case)

@kraih kraih merged commit 7872115 into master Sep 11, 2019
@rohanc
Copy link

rohanc commented Sep 13, 2019

Just FYI, these changes mean that modules like MojoX::Log::Log4perl will need to be updated.

I'm not expecting the Mojolicious team to fix it -- just thought you might like to know.
https://rt.cpan.org/Public/Bug/Display.html?id=130499

@kraih kraih deleted the log_context branch November 6, 2019 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcusramberg marcusramberg marcusramberg approved these changes

@jberger jberger jberger approved these changes

@christopherraa christopherraa christopherraa approved these changes

@CandyAngel CandyAngel CandyAngel approved these changes

@Grinnz Grinnz Awaiting requested review from Grinnz

@jhthorsen jhthorsen Awaiting requested review from jhthorsen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kraih @jberger @rohanc @marcusramberg @jhthorsen @christopherraa @CandyAngel