Skip to content

Add a scoped PSR logger for apps #21874

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
Merged
merged 1 commit into from
Jul 16, 2020
Merged

Add a scoped PSR logger for apps #21874

merged 1 commit into from
Jul 16, 2020

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Jul 16, 2020 •
edited
Loading

Just like for ILogger we should have a version that has the app ID
pre-set for the context (unless overwritten) so that each log entry can
be traced back to the app that produced it.

This adds a decorator that does just this.

Next step will be to deprecate ILogger 😈 🔥 💣

@ChristophWurst
Add a scoped PSR logger for apps
c889021 
Just like for ILogger we should have a version that has the app ID
pre-set for the context (unless overwritten) so that each log entry can
be traced back to the app that produced it.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst added this to the Nextcloud 20 milestone Jul 16, 2020
@ChristophWurst ChristophWurst self-assigned this Jul 16, 2020
@ChristophWurst ChristophWurst mentioned this pull request Jul 16, 2020
Merged
kesselb
kesselb approved these changes Jul 16, 2020
View reviewed changes
Copy link
Collaborator

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

LGTM. I find array_merge a bit heavy given that the first array is always the same. !isset($context[app]) add app id would work as well.

@ChristophWurst
Copy link
Member Author

LGTM. I find array_merge a bit heavy given that the first array is always the same. !isset($context[app]) add app id would work as well.

Is that really slower? I've tried to test this a bit at https://3v4l.org/Y9Wd0/perf#output but the isset version was even a tad slower.

@kesselb
Copy link
Collaborator

kesselb commented Jul 16, 2020

LGTM. I find array_merge a bit heavy given that the first array is always the same. !isset($context[app]) add app id would work as well.

Is that really slower? I've tried to test this a bit at https://3v4l.org/Y9Wd0/perf#output but the isset version was even a tad slower.

I don't have a strong opinion on that ;) But your benchmark shows that isset is faster 🤔 (I usually add system and user time as per https://stackoverflow.com/a/556411).

@rullzer rullzer merged commit 3085cf7 into master Jul 16, 2020
@rullzer rullzer deleted the feature/scoped-psr-logger branch July 16, 2020 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzer rullzer rullzer approved these changes

@kesselb kesselb kesselb approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

@julien-nc julien-nc Awaiting requested review from julien-nc

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees

@ChristophWurst ChristophWurst

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Milestone

Nextcloud 20

Development

Successfully merging this pull request may close these issues.

4 participants

@ChristophWurst @kesselb @rullzer