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

Add basic optional sentry.io integration #4632

Merged
merged 7 commits into from Feb 18, 2019

Conversation

Projects
None yet
4 participants
@erikjohnston
Copy link
Member

erikjohnston commented Feb 12, 2019

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 error or above, plus the last few log lines for context (called "breadcrumbs").

Future extensions:

  • Only upload log lines from the same logging context (I have a patch that does this, but probably needs some cleaning up).
  • Fill out the context a bit more, things like authenticated user and remote IP are recommended.

erikjohnston added some commits Feb 12, 2019

@erikjohnston erikjohnston requested review from matrix-org/synapse-core and michaelkaye Feb 12, 2019

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Feb 12, 2019

(cc @michaelkaye in case he has some insights into best practices)

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 12, 2019

Codecov Report

Merging #4632 into develop will increase coverage by 0.01%.
The diff coverage is 29.16%.

@@             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
@michaelkaye
Copy link
Contributor

michaelkaye left a 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.

Show resolved Hide resolved synapse/app/_base.py Outdated
Show resolved Hide resolved synapse/config/metrics.py Outdated
Show resolved Hide resolved synapse/config/metrics.py Outdated

@erikjohnston erikjohnston force-pushed the erikj/basic_sentry branch from 4cf8d22 to 6cb415b Feb 13, 2019

@@ -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"],

This comment has been minimized.

@richvdh

richvdh Feb 18, 2019

Member

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.

This comment has been minimized.

@erikjohnston

erikjohnston Feb 18, 2019

Author Member

I think I somewhat agree, though I think that is a change to consider outside of this PR

This comment has been minimized.

@richvdh
Show resolved Hide resolved synapse/config/metrics.py Outdated
Show resolved Hide resolved synapse/config/metrics.py Outdated
Show resolved Hide resolved synapse/config/metrics.py
@@ -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)

This comment has been minimized.

@richvdh

richvdh Feb 18, 2019

Member

doesn't this want to be earlier?

This comment has been minimized.

@erikjohnston

erikjohnston Feb 18, 2019

Author Member

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.

erikjohnston added some commits Feb 18, 2019

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Feb 18, 2019

@erikjohnston erikjohnston merged commit d154f5a into develop Feb 18, 2019

7 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
codecov/patch 29.16% of diff hit (target 0%)
Details
codecov/project 75.29% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@erikjohnston erikjohnston deleted the erikj/basic_sentry branch Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.