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

Add a mechanism to register crash reporters #7151

Merged
merged 1 commit into from Nov 14, 2017

Conversation

@ChristophWurst
Member

ChristophWurst commented Nov 12, 2017

This adds a crash reporter registry, which is meant for third party apps
to hook into the error logging/reporting chain. This way, external tools
like Sentry can be used to track and aggregate system crashes.

Out of personal interest (I'm using Sentry to monitor some Laravel apps), I looked into integrating Sentry into Nextcloud. As a first attempt, I just used the provided SDK and registered error/exception handlers. While this works for some triggered errors, it does not report and exception as we catch and handle them in places like index.php. Thus, I needed to find a way to hook into the exception reporting chain and found out that our logger is usually called to log the exception. Therefore, I've added a little mechanism that allows apps to register any reporter that implements a certain interface. If no reporters are registered, the registry doesn't do anything.

Usually, I would have opened a ticket first to discuss the general idea but since it's a rainy Sunday afternoon and I was motivated to invest some time on this, I started the implementation right away and it was finished quickly.

Let me know what you think. The world doesn't end if anyone feels like this is too specific and we shouldn't add this to our public API. After all, someone has to maintain it. Still, I think it's a nice little enhancement and used correctly, the usage of Sentry will help detect more unnoticed errors that usually get lost in log files.

If we decide to integrate this I have to add some tests. done

Steps to test

cc @LukasReschke @nickvergessen @rullzer

Show outdated Hide outdated lib/private/Log.php Outdated
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 12, 2017

Codecov Report

Merging #7151 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #7151      +/-   ##
============================================
+ Coverage     50.75%   50.75%   +<.01%     
- Complexity    24479    24483       +4     
============================================
  Files          1581     1582       +1     
  Lines         93578    93589      +11     
  Branches       1353     1353              
============================================
+ Hits          47493    47502       +9     
- Misses        46085    46087       +2
Impacted Files Coverage Δ Complexity Δ
lib/private/Log.php 78.88% <100%> (-0.43%) 39 <4> (+1)
lib/private/Server.php 83.35% <100%> (-0.07%) 125 <0> (ø)
lib/private/Support/CrashReport/Registry.php 100% <100%> (ø) 3 <3> (?)
apps/files_trashbin/lib/Expiration.php 90.32% <0%> (-1.62%) 29% <0%> (ø)
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php 72.53% <0%> (+0.24%) 136% <0%> (ø) ⬇️
lib/private/Security/CertificateManager.php 92.07% <0%> (+0.99%) 39% <0%> (ø) ⬇️

codecov bot commented Nov 12, 2017

Codecov Report

Merging #7151 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #7151      +/-   ##
============================================
+ Coverage     50.75%   50.75%   +<.01%     
- Complexity    24479    24483       +4     
============================================
  Files          1581     1582       +1     
  Lines         93578    93589      +11     
  Branches       1353     1353              
============================================
+ Hits          47493    47502       +9     
- Misses        46085    46087       +2
Impacted Files Coverage Δ Complexity Δ
lib/private/Log.php 78.88% <100%> (-0.43%) 39 <4> (+1)
lib/private/Server.php 83.35% <100%> (-0.07%) 125 <0> (ø)
lib/private/Support/CrashReport/Registry.php 100% <100%> (ø) 3 <3> (?)
apps/files_trashbin/lib/Expiration.php 90.32% <0%> (-1.62%) 29% <0%> (ø)
lib/private/Files/Cache/Propagator.php 94.93% <0%> (-1.27%) 16% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php 72.53% <0%> (+0.24%) 136% <0%> (ø) ⬇️
lib/private/Security/CertificateManager.php 92.07% <0%> (+0.99%) 39% <0%> (ø) ⬇️

@ChristophWurst ChristophWurst added this to IN PROGRESS (max 3 PRs) in Christoph's Tasks Nov 12, 2017

@skjnldsv

This comment has been minimized.

Show comment
Hide comment
@skjnldsv

skjnldsv Nov 13, 2017

Member

I don't really know specifically what are the full possibilities of sentry, but if I understand correctly, will this report automatically errors from users in the background?
Anonymously? :)

Member

skjnldsv commented Nov 13, 2017

I don't really know specifically what are the full possibilities of sentry, but if I understand correctly, will this report automatically errors from users in the background?
Anonymously? :)

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst Nov 13, 2017

Member

I don't really know specifically what are the full possibilities of sentry, but if I understand correctly, will this report automatically errors from users in the background?

Exactly. Crashes (unhandled exceptions and errors) are sent to Sentry which collects and aggregates all reports, assigns them to users/releases and so on. This allows you to assign people to those errors and detect regressions.

bildschirmfoto von 2017-11-13 10-32-45

Anonymously? :)

No, you have to either have an account on the hosted version or set it up yourself.

Member

ChristophWurst commented Nov 13, 2017

I don't really know specifically what are the full possibilities of sentry, but if I understand correctly, will this report automatically errors from users in the background?

Exactly. Crashes (unhandled exceptions and errors) are sent to Sentry which collects and aggregates all reports, assigns them to users/releases and so on. This allows you to assign people to those errors and detect regressions.

bildschirmfoto von 2017-11-13 10-32-45

Anonymously? :)

No, you have to either have an account on the hosted version or set it up yourself.

@skjnldsv

This comment has been minimized.

Show comment
Hide comment
@skjnldsv

skjnldsv Nov 13, 2017

Member

Okay, so we can't just gather data from our users to report errors? (aside from the fact that those data could be sensitive and from the ethical view of this)

Looks promising either way! :)

Member

skjnldsv commented Nov 13, 2017

Okay, so we can't just gather data from our users to report errors? (aside from the fact that those data could be sensitive and from the ethical view of this)

Looks promising either way! :)

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst Nov 13, 2017

Member

Okay, so we can't just gather data from our users to report errors? (aside from the fact that those data could be sensitive and from the ethical view of this)

Sure, we could do that. But I assume that takes some engineering power to do properly to actually gain anything from the anonymous reports ;)

Member

ChristophWurst commented Nov 13, 2017

Okay, so we can't just gather data from our users to report errors? (aside from the fact that those data could be sensitive and from the ethical view of this)

Sure, we could do that. But I assume that takes some engineering power to do properly to actually gain anything from the anonymous reports ;)

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Nov 13, 2017

Member

great idea! 👍

Member

karlitschek commented Nov 13, 2017

great idea! 👍

@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst Nov 13, 2017

Member

I've added some tests. This should be ready to review :)

Member

ChristophWurst commented Nov 13, 2017

I've added some tests. This should be ready to review :)

@MorrisJobke

Tested, works and the code looks good 👍

Add a mechanism to register crach reporters
This adds a crash reporter registry, which is meant for third party apps
to hook into the error logging/reporting chain. This way, external tools
like Sentry can be used to track and aggregate system crashes.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst

This comment has been minimized.

Show comment
Hide comment
@ChristophWurst

ChristophWurst Nov 14, 2017

Member

Tested, works and the code looks good 👍

\o/

I autosquashed the fixup commits and rebased onto master.

Member

ChristophWurst commented Nov 14, 2017

Tested, works and the code looks good 👍

\o/

I autosquashed the fixup commits and rebased onto master.

@ChristophWurst ChristophWurst moved this from IN PROGRESS (max 3 PRs) to TO REVIEW (max 4 PRs) in Christoph's Tasks Nov 14, 2017

@LukasReschke LukasReschke merged commit 48710a6 into master Nov 14, 2017

4 checks passed

Scrutinizer 8 updated code elements
Details
codecov/patch 100% of diff hit (target 50.75%)
Details
codecov/project 50.75% (+<.01%) compared to 28e8a1c
Details
continuous-integration/drone/pr the build was successful
Details

Christoph's Tasks automation moved this from TO REVIEW (max 4 PRs) to DONE Nov 14, 2017

@LukasReschke LukasReschke deleted the feature/crash-reporter-registry branch Nov 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment