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

Diagnostics event logging to log #31124

Merged
merged 5 commits into from
Mar 2, 2022
Merged

Diagnostics event logging to log #31124

merged 5 commits into from
Mar 2, 2022

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Feb 11, 2022 •
edited
Loading

  • Add diagnostics.logging to enable timing events being logged to the Nextcloud log at debug level
  • log.condition can be used in order to limit the log generation to wished conditions
  • diagnostics.logging.theshold allows to only log messages taking longer than the configured number of milliseconds
  • Additional events added:
    • controller execution
    • app loading
    • Bootstrap register/boot
    • Time to connect to the database
    • Time to connect/auth/select the redis db

With this it is more easily possible to understand certain execution paths of slow requests on systems where a profiler cannot easily be attached.

@juliushaertl
Copy link
Member Author

@CarlSchwan This may have similar use cases as the debugger you started working on but having this logged in some way could be useful in some scenarios where we cannot issue the requests ourselves.

@juliushaertl
Copy link
Member Author

@CarlSchwan @PVince81 @nickvergessen Any thoughts about this? I'd extend the cases where the event logger is used (e.g. for webdav) further if there are no concerns about the general approach to get some deeper insight into systems using that approach.

CarlSchwan
CarlSchwan reviewed Feb 11, 2022
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

That seems like a good idea, in particular having more EventLogger is particularly helpful also for my profiler branch as they then get displayed in the debug toolbar.

lib/private/AppFramework/Bootstrap/Coordinator.php Outdated Show resolved Hide resolved
@juliushaertl juliushaertl force-pushed the enh/diagnostics-logging branch 2 times, most recently from 8834aef to f70cbac Compare February 25, 2022 15:19
@juliushaertl juliushaertl marked this pull request as ready for review February 25, 2022 15:20
@juliushaertl juliushaertl added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 25, 2022
@juliushaertl juliushaertl requested review from a team, nickvergessen, PVince81 and CarlSchwan and removed request for a team February 25, 2022 15:29
@juliushaertl juliushaertl added this to the Nextcloud 24 milestone Feb 25, 2022
@PVince81 PVince81 requested a review from Pytal February 25, 2022 16:15
Pytal
Pytal approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

🚀

config/config.sample.php Outdated Show resolved Hide resolved
lib/base.php Show resolved Hide resolved
@juliushaertl
Diagnostics event logging to Nextcloud log
0f33453 
Signed-off-by: Julius Härtl <[email protected]>

Add config samples

Signed-off-by: Julius Härtl <[email protected]>
@juliushaertl
Add event logging to app loading
eede608 
Signed-off-by: Julius Härtl <[email protected]>
@juliushaertl
Fix tests
2dd96fe 
Signed-off-by: Julius Härtl <[email protected]>
@juliushaertl
Add logging around dav exec
f13c150 
Signed-off-by: Julius Härtl <[email protected]>
@juliushaertl
Add event logging for db and redis connection
83717a7 
Signed-off-by: Julius Härtl <[email protected]>
@PVince81
Copy link
Member

PVince81 commented Mar 2, 2022

@juliushaertl please merge if your force push did not change anything after the existing reviews (it's green!)

should we backport this to be able to do diagnostics in earlier major versions ?

@juliushaertl
Copy link
Member Author

👍 Only changed the typo in the config documentation and squashed commits afterwards

@juliushaertl juliushaertl merged commit 2ff0c97 into master Mar 2, 2022
@juliushaertl juliushaertl deleted the enh/diagnostics-logging branch March 2, 2022 11:00
@juliushaertl
Copy link
Member Author

/backport to stable23

@juliushaertl
Copy link
Member Author

/backport to stable22

@juliushaertl
Copy link
Member Author

/backport to stable21

This was referenced Mar 2, 2022
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@Pytal Pytal Pytal approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement performance 🚀 technical debt
Projects
None yet
Milestone
Nextcloud 24
Development

Successfully merging this pull request may close these issues.
5 participants
@juliushaertl @PVince81 @nickvergessen @CarlSchwan @Pytal