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

Ability to log extra fields + trigger event when logging #31135

Merged

Conversation

DeepDiver1975
Copy link
Member

*port of #31121

@DeepDiver1975 DeepDiver1975 added this to the development milestone Apr 16, 2018
@DeepDiver1975 DeepDiver1975 self-assigned this Apr 16, 2018
@DeepDiver1975 DeepDiver1975 changed the title Master 7ff1236ec41bfd74ccd92762442072113e7b3f61 Ability to log extra fields + trigger event when logging Apr 16, 2018
$skipEvents = true;
}

$formattedMessage = $this->interpolate($message, $context);
Copy link
Member

Choose a reason for hiding this comment

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

interpolation should only happen after the event has been handled. The app might want to change the context before interpolating. move this inside the if ($level >= $minLevel) {

Copy link
Member Author

Choose a reason for hiding this comment

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

see my comment on the original PR of this event dispatching of log messages.
the performance optimization is obsolete because the formatted message is part of the event which is dispatched in any case.

Copy link
Member

Choose a reason for hiding this comment

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

Any hook listener should redo the interpolation

Copy link
Contributor

Choose a reason for hiding this comment

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

We'd then need to expose the interpolation method to hook listeners.

I agree with @DeepDiver1975 and would like to move forward and merge this

Copy link
Member Author

Choose a reason for hiding this comment

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

redoing the interpolation on all listeners will duplicate the calls - not optimal either.

@DeepDiver1975
Copy link
Member Author

@ownclouders rebase

@ownclouders
Copy link
Contributor

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@ownclouders
Copy link
Contributor

Automated rebase with GitMate.io was successful! 🎉

@ownclouders ownclouders force-pushed the master-7ff1236ec41bfd74ccd92762442072113e7b3f61 branch from ae05fdc to 2d5f19d Compare April 19, 2018 12:55
@codecov
Copy link

codecov bot commented Apr 19, 2018

Codecov Report

Merging #31135 into master will increase coverage by 0.01%.
The diff coverage is 94.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #31135      +/-   ##
============================================
+ Coverage      62.6%   62.61%   +0.01%     
- Complexity    18253    18262       +9     
============================================
  Files          1145     1145              
  Lines         68566    68598      +32     
  Branches       1234     1234              
============================================
+ Hits          42925    42955      +30     
- Misses        25280    25282       +2     
  Partials        361      361
Flag Coverage Δ Complexity Δ
#javascript 52.05% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 63.81% <94.44%> (+0.01%) 18262 <17> (+9) ⬆️
Impacted Files Coverage Δ Complexity Δ
lib/private/Log/Owncloud.php 77.96% <75%> (-0.22%) 18 <12> (+2)
lib/private/Log.php 81.03% <96.87%> (+4.89%) 47 <5> (+7) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b36c56...6a11a90. Read the comment docs.

@PVince81
Copy link
Contributor

added commit from #31247

@PVince81
Copy link
Contributor

we need to finish this to avoid discrepancy with stable10

should we get rid of the optimization ?

@tomneedham
Copy link
Contributor

IMO remove optimisation - separate PR, target master then backport, like usual. Now we're messing with a forward port and it's going to end in tears.

@DeepDiver1975 DeepDiver1975 force-pushed the master-7ff1236ec41bfd74ccd92762442072113e7b3f61 branch from c7855de to 1acfbac Compare May 11, 2018 09:50
Vincent Petry added 2 commits May 17, 2018 15:27
If a logger supports it like the default logger, additional fields can
now be added to the base JSON object.
@DeepDiver1975 DeepDiver1975 force-pushed the master-7ff1236ec41bfd74ccd92762442072113e7b3f61 branch from 1acfbac to 6a11a90 Compare May 17, 2018 13:31
@patrickjahns
Copy link
Contributor

@DeepDiver1975 @PVince81 - any update?

@PVince81
Copy link
Contributor

PVince81 commented Jun 11, 2018

blocks forward port of #31623 ([stable10][feature] provide original exception via logging events)

@DeepDiver1975
Copy link
Member Author

merging this to move on

@DeepDiver1975 DeepDiver1975 merged commit b7b2f6e into master Jul 3, 2018
@DeepDiver1975 DeepDiver1975 deleted the master-7ff1236ec41bfd74ccd92762442072113e7b3f61 branch July 3, 2018 07:12
@lock
Copy link

lock bot commented Jul 30, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants