Add basic optional sentry.io integration #4632
Conversation
|
(cc @michaelkaye in case he has some insights into best practices) |
Codecov Report
@@ Coverage Diff @@
## develop #4632 +/- ##
===========================================
+ Coverage 75.28% 75.29% +0.01%
===========================================
Files 338 338
Lines 34579 34793 +214
Branches 5655 5722 +67
===========================================
+ Hits 26032 26198 +166
- Misses 6957 6989 +32
- Partials 1590 1606 +16 |
There was a problem hiding this comment.
So the code will work, but I think we need to be much more verbose about the impact this has on private data - there are no known issues sending data from an application to a sentry instance or within the sentry instance; but it does have issues if there are notifications on from a sentry instance to email.
If we attach too much data to these requests, that data may leak onwards, and we may end up with credentials in unencrypted email histories, which would be bad.
https://docs.sentry.io/data-management/sensitive-data/ provides their information on scrubbing, but we should not depend on server-side scrubbing to make this safe (we do not know that every synapse admin will be using sentry correctly) - so we should prevent the flow of information out from synapse in the first place.
I think this is fair to merge as is, for the limited use cases when the administrator is aware of what they're doing, and takes responsibility for correctly configuring their sentry instance; but i'd give it a lot of health warnings for those without that experience until we perform the filtering required to limit the sending of data.
(We also need to commit to ensuring we continue to update those filters as required to prevent any new sensitive information from being leaked)
I am very in favour of deploying this as it's an amazing tool to help diagnose issues in production systems - we just need to make sure the filters are correct to only expose what's going on, so we can get "lots of errors at line 621 when you set the send_to_all_servers flag to True", but not any secrets that go alongside that.
erikjohnston
force-pushed
the
erikj/basic_sentry
branch
from
4cf8d22 to
6cb415b
Compare
| @@ -86,6 +86,7 @@ | |||
| "saml2": ["pysaml2>=4.5.0"], | |||
| "url_preview": ["lxml>=3.5.0"], | |||
| "test": ["mock>=2.0", "parameterized"], | |||
| "sentry": ["sentry-sdk>=0.7.2"], | |||
There was a problem hiding this comment.
I'm a bit concerned that we tell people to install with synapse[all], and it's starting to pull in the entire internet. Perhaps we should consider revising that advice.
There was a problem hiding this comment.
I think I somewhat agree, though I think that is a change to consider outside of this PR
| @@ -266,9 +268,37 @@ def handle_sighup(*args, **kwargs): | |||
| # It is now safe to start your Synapse. | |||
| hs.start_listening(listeners) | |||
| hs.get_datastore().start_profiling() | |||
|
|
|||
| setup_sentry(hs) | |||
There was a problem hiding this comment.
doesn't this want to be earlier?
There was a problem hiding this comment.
I'd expect any errors to happen before then to be fatal configuration errors, which I don't think we'd want to bother reporting to sentry, so I don't think it really matters too much where in the setup code we initialise it.
I've found sentry.io quite useful to see what errors synapse is producing on jki.re, so I think its worth adding. This is very bare-bones, but is enough to be useful.
By default, sentry will upload all records logged at
erroror above, plus the last few log lines for context (called "breadcrumbs").Future extensions: