Skip to content

Comments

Add a scoped PSR logger for apps#21874

Merged
rullzer merged 1 commit intomasterfrom
feature/scoped-psr-logger
Jul 16, 2020
Merged

Add a scoped PSR logger for apps#21874
rullzer merged 1 commit intomasterfrom
feature/scoped-psr-logger

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 <[email protected]>
@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
Contributor

@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

ChristophWurst 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.

@kesselb
Copy link
Contributor

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.

3 participants

@ChristophWurst @kesselb @rullzer