From 28511e262e227afc0d3dd3ffdb13d267633e17c2 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 28 Sep 2021 09:34:26 +0200 Subject: [PATCH] Use new sentry-sdk and adapt usage pattern Replaces our custom Sentry WSGI middleware with middleware shipped by sentry-sdk. Adds sentry-sdk ASGI middleware. Our custom sentry middleware also allowed logging slow requests to sentry. I have dropped this for now, and if we still want this in the future, we should probably implement this as a custom logging middleware instead, which would automatically appear in Sentry as well. We used to hardcode the log level for events to WARNING, and the Sentry default is ERROR now, which I think is a good idea. I've made this configurable using the sentry_event_level config variable. It does not seem to be possible to connect multiple dsn's in the same app, unless we start hacking (https://github.com/getsentry/sentry-python/issues/198), so I dropped the custom_dsn from the sentry error reporting plugin (which has been broken anyway, so shouldn't be a big deal). --- doc/source/admin/galaxy_options.rst | 18 +-- lib/galaxy/app.py | 16 +- lib/galaxy/config/__init__.py | 11 +- .../config/sample/error_report.yml.sample | 5 +- lib/galaxy/config/sample/galaxy.yml.sample | 9 +- lib/galaxy/dependencies/__init__.py | 2 +- .../dependencies/conditional-requirements.txt | 2 +- .../tools/error_reports/plugins/sentry.py | 146 +++++++++--------- lib/galaxy/web/framework/middleware/sentry.py | 138 ----------------- lib/galaxy/webapps/galaxy/buildapp.py | 5 +- lib/galaxy/webapps/galaxy/config_schema.yml | 13 +- lib/galaxy/webapps/galaxy/fast_app.py | 5 + 12 files changed, 111 insertions(+), 259 deletions(-) delete mode 100644 lib/galaxy/web/framework/middleware/sentry.py diff --git a/doc/source/admin/galaxy_options.rst b/doc/source/admin/galaxy_options.rst index 8c021c3c3ca..18dbd560445 100644 --- a/doc/source/admin/galaxy_options.rst +++ b/doc/source/admin/galaxy_options.rst @@ -2788,18 +2788,16 @@ :Type: str -~~~~~~~~~~~~~~~~~~~~~~~~~~~ -``sentry_sloreq_threshold`` -~~~~~~~~~~~~~~~~~~~~~~~~~~~ +~~~~~~~~~~~~~~~~~~~~~~ +``sentry_event_level`` +~~~~~~~~~~~~~~~~~~~~~~ :Description: - Sentry slow request logging. Requests slower than the threshold - indicated below will be sent as events to the configured Sentry - server (above, sentry_dsn). A value of '0' is disabled. For - example, you would set this to .005 to log all queries taking - longer than 5 milliseconds. -:Default: ``0.0`` -:Type: float + Determines the minimum log level that will be sent as an event to + Sentry. Possible values are DEBUG, INFO, WARNING, ERROR or + CRITICAL. +:Default: ``ERROR`` +:Type: str ~~~~~~~~~~~~~~~ diff --git a/lib/galaxy/app.py b/lib/galaxy/app.py index 58b1eea05ea..65287408032 100644 --- a/lib/galaxy/app.py +++ b/lib/galaxy/app.py @@ -193,10 +193,22 @@ class GalaxyManagerApplication(MinimalManagerApp, MinimalGalaxyApplication): self.sentry_client = None if self.config.sentry_dsn: + event_level = self.config.sentry_event_level.upper() + assert event_level in ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL'], f"Invalid sentry event level '{self.config.sentry.event_level}'" def postfork_sentry_client(): - import raven - self.sentry_client = raven.Client(self.config.sentry_dsn, transport=raven.transport.HTTPTransport) + import sentry_sdk + from sentry_sdk.integrations.logging import LoggingIntegration + + sentry_logging = LoggingIntegration( + level=logging.INFO, # Capture info and above as breadcrumbs + event_level=getattr(logging, event_level) # Send errors as events + ) + self.sentry_client = sentry_sdk.init( + self.config.sentry_dsn, + release=f"{self.config.version_major}.{self.config.version_minor}", + integrations=[sentry_logging] + ) self.application_stack.register_postfork_function(postfork_sentry_client) diff --git a/lib/galaxy/config/__init__.py b/lib/galaxy/config/__init__.py index 2ce875e1aa2..6b2a0a693d1 100644 --- a/lib/galaxy/config/__init__.py +++ b/lib/galaxy/config/__init__.py @@ -45,10 +45,7 @@ from galaxy.util.properties import ( running_from_source, ) from galaxy.web.formatting import expand_pretty_datetime_format -from galaxy.web_stack import ( - get_stack_facts, - register_postfork_function -) +from galaxy.web_stack import get_stack_facts from ..version import VERSION_MAJOR, VERSION_MINOR log = logging.getLogger(__name__) @@ -1106,7 +1103,6 @@ def configure_logging(config): """ # Get root logger logging.addLevelName(LOGLV_TRACE, "TRACE") - root = logging.getLogger() # PasteScript will have already configured the logger if the # 'loggers' section was found in the config file, otherwise we do # some simple setup using the 'log_*' values from the config. @@ -1129,11 +1125,6 @@ def configure_logging(config): conf['filename'] = conf.pop('filename_template').format(**get_stack_facts(config=config)) logging_conf['handlers'][name] = conf logging.config.dictConfig(logging_conf) - if getattr(config, "sentry_dsn", None): - from raven.handlers.logging import SentryHandler - sentry_handler = SentryHandler(config.sentry_dsn) - sentry_handler.setLevel(logging.WARN) - register_postfork_function(root.addHandler, sentry_handler) class ConfiguresGalaxyMixin: diff --git a/lib/galaxy/config/sample/error_report.yml.sample b/lib/galaxy/config/sample/error_report.yml.sample index 193e5b2b1be..8a2ada8e8eb 100644 --- a/lib/galaxy/config/sample/error_report.yml.sample +++ b/lib/galaxy/config/sample/error_report.yml.sample @@ -15,7 +15,7 @@ # verbose/user_submission, but those are not necessary to provide. # The default Email bug reporter. By default, the standard -# configuration is taken from your galaxy.ini +# configuration is taken from your galaxy.yml - type: email verbose: true user_submission: true @@ -29,8 +29,7 @@ # directory: /tmp/reports/ # Submit error reports to sentry. If a sentry_dsn is configured in your -# galaxy.ini, then Galaxy will submit the job error to Sentry. You may supply a -# separate DSN for tool reports by supplying a ``custom_dsn`` parameter. +# galaxy.yml, then Galaxy will submit the job error to Sentry. - type: sentry user_submission: false diff --git a/lib/galaxy/config/sample/galaxy.yml.sample b/lib/galaxy/config/sample/galaxy.yml.sample index 75ea984d432..43c57775245 100644 --- a/lib/galaxy/config/sample/galaxy.yml.sample +++ b/lib/galaxy/config/sample/galaxy.yml.sample @@ -1388,12 +1388,9 @@ galaxy: # -> Settings -> API Keys. #sentry_dsn: null - # Sentry slow request logging. Requests slower than the threshold - # indicated below will be sent as events to the configured Sentry - # server (above, sentry_dsn). A value of '0' is disabled. For - # example, you would set this to .005 to log all queries taking longer - # than 5 milliseconds. - #sentry_sloreq_threshold: 0.0 + # Determines the minimum log level that will be sent as an event to + # Sentry. Possible values are DEBUG, INFO, WARNING, ERROR or CRITICAL. + #sentry_event_level: ERROR # Log to statsd Statsd is an external statistics aggregator # (https://github.com/etsy/statsd) Enabling the following options will diff --git a/lib/galaxy/dependencies/__init__.py b/lib/galaxy/dependencies/__init__.py index aab3f19988f..52e9ec331e3 100644 --- a/lib/galaxy/dependencies/__init__.py +++ b/lib/galaxy/dependencies/__init__.py @@ -191,7 +191,7 @@ class ConditionalDependencies: def check_fluent_logger(self): return asbool(self.config["fluent_log"]) - def check_raven(self): + def check_sentry_sdk(self): return self.config.get("sentry_dsn", None) is not None def check_statsd(self): diff --git a/lib/galaxy/dependencies/conditional-requirements.txt b/lib/galaxy/dependencies/conditional-requirements.txt index a1accf07738..0a139922079 100644 --- a/lib/galaxy/dependencies/conditional-requirements.txt +++ b/lib/galaxy/dependencies/conditional-requirements.txt @@ -2,7 +2,7 @@ psycopg2-binary==2.8.4 mysqlclient fluent-logger -raven +sentry-sdk pbs_python drmaa statsd diff --git a/lib/galaxy/tools/error_reports/plugins/sentry.py b/lib/galaxy/tools/error_reports/plugins/sentry.py index 59f578232ac..f99ea963fc0 100644 --- a/lib/galaxy/tools/error_reports/plugins/sentry.py +++ b/lib/galaxy/tools/error_reports/plugins/sentry.py @@ -1,12 +1,18 @@ """The module describes the ``sentry`` error plugin plugin.""" import logging +try: + import sentry_sdk +except ImportError: + sentry_sdk = None + from galaxy import web -from galaxy.util import string_as_bool, unicodify +from galaxy.util import string_as_bool from . import ErrorPlugin log = logging.getLogger(__name__) +SENTRY_SDK_IMPORT_MESSAGE = 'The Python sentry-sdk package is required to use this feature, please install it' ERROR_TEMPLATE = """Galaxy Job Error: {tool_id} v{tool_version} Command Line: @@ -32,94 +38,80 @@ class SentryPlugin(ErrorPlugin): self.redact_user_details_in_bugreport = self.app.config.redact_user_details_in_bugreport self.verbose = string_as_bool(kwargs.get('verbose', False)) self.user_submission = string_as_bool(kwargs.get('user_submission', False)) - self.custom_dsn = kwargs.get('custom_dsn', None) - self.sentry = None - # Use the built in one by default - if hasattr(self.app, 'sentry_client'): - self.sentry = self.app.sentry_client - - # if they've set a custom one, override. - if self.custom_dsn: - import raven - self.sentry = raven.Client(self.custom_dsn, transport=raven.transport.HTTPTransport) + assert sentry_sdk, SENTRY_SDK_IMPORT_MESSAGE def submit_report(self, dataset, job, tool, **kwargs): """Submit the error report to sentry """ - if self.sentry: - user = job.get_user() - extra = { - 'info': job.info, - 'id': job.id, - 'command_line': unicodify(job.command_line), - 'destination_id': unicodify(job.destination_id), - 'stderr': unicodify(job.stderr), - 'traceback': unicodify(job.traceback), - 'exit_code': job.exit_code, - 'stdout': unicodify(job.stdout), - 'handler': unicodify(job.handler), - 'tool_id': unicodify(job.tool_id), - 'tool_version': unicodify(job.tool_version), - 'tool_xml': unicodify(tool.config_file) if tool else None - } - if self.redact_user_details_in_bugreport: - extra['email'] = 'redacted' - else: - if 'email' in kwargs: - extra['email'] = unicodify(kwargs['email']) + extra = { + 'info': job.info, + 'id': job.id, + 'command_line': job.command_line, + 'destination_id': job.destination_id, + 'stderr': job.stderr, + 'traceback': job.traceback, + 'exit_code': job.exit_code, + 'stdout': job.stdout, + 'handler': job.handler, + 'tool_id': job.tool_id, + 'tool_version': job.tool_version, + 'tool_xml': tool.config_file if tool else None + } + if self.redact_user_details_in_bugreport: + extra['email'] = 'redacted' + else: + if 'email' in kwargs: + extra['email'] = kwargs['email'] - # User submitted message - extra['message'] = unicodify(kwargs.get('message', '')) + # User submitted message + extra['message'] = kwargs.get('message', '') - # Construct the error message to send to sentry. The first line - # will be the issue title, everything after that becomes the - # "message" - error_message = ERROR_TEMPLATE.format(**extra) + # Construct the error message to send to sentry. The first line + # will be the issue title, everything after that becomes the + # "message" + error_message = ERROR_TEMPLATE.format(**extra) - # Update context with user information in a sentry-specific manner - context = {} + # Update context with user information in a sentry-specific manner + context = {} - # Getting the url allows us to link to the dataset info page in case - # anything is missing from this report. - try: - url = web.url_for(controller="dataset", - action="details", - dataset_id=self.app.security.encode_id(dataset.id), - qualified=True) - except AttributeError: - # The above does not work when handlers are separate from the web handlers - url = None + # Getting the url allows us to link to the dataset info page in case + # anything is missing from this report. + try: + url = web.url_for(controller="dataset", + action="show_params", + dataset_id=self.app.security.encode_id(dataset.id), + qualified=True) + except AttributeError: + # The above does not work when handlers are separate from the web handlers + url = None - if self.redact_user_details_in_bugreport: - if user: - # Opauqe identifier - context['user'] = { - 'id': user.id - } - else: - if user: - # User information here also places email links + allows seeing - # a list of affected users in the tags/filtering. - context['user'] = { - 'name': user.username, - 'email': user.email, - } + user = job.get_user() + if self.redact_user_details_in_bugreport: + if user: + # Opaque identifier + context['user'] = { + 'id': user.id + } + else: + if user: + # User information here also places email links + allows seeing + # a list of affected users in the tags/filtering. + context['user'] = { + 'name': user.username, + 'email': user.email, + } - context['request'] = {'url': url} + context['request'] = {'url': url} - self.sentry_client.context.merge(context) + for key, value in context.items(): + sentry_sdk.set_context(key, value) + sentry_sdk.set_context('job', extra) + sentry_sdk.set_tag('tool_id', job.tool_id) + sentry_sdk.set_tag('tool_version', job.tool_version) - # Send the message, using message because - response = self.sentry_client.capture( - 'raven.events.Message', - tags={ - 'tool_id': job.tool_id, - 'tool_version': job.tool_version, - }, - extra=extra, - message=unicodify(error_message), - ) - return (f'Submitted bug report to Sentry. Your guru meditation number is {response}', 'success') + # Send the message, using message because + response = sentry_sdk.capture_message(error_message) + return (f'Submitted bug report to Sentry. Your guru meditation number is {response}', 'success') __all__ = ('SentryPlugin', ) diff --git a/lib/galaxy/web/framework/middleware/sentry.py b/lib/galaxy/web/framework/middleware/sentry.py deleted file mode 100644 index 254190a4322..00000000000 --- a/lib/galaxy/web/framework/middleware/sentry.py +++ /dev/null @@ -1,138 +0,0 @@ -""" -raven.middleware -~~~~~~~~~~~~~~~~~~~~~~~~ - -:copyright: (c) 2010-2012 by the Sentry Team, see AUTHORS for more details. -:license: BSD, see LICENSE for more details. -""" -import time - -try: - from raven import Client - from raven.utils.wsgi import get_current_url, get_headers, get_environ -except ImportError: - Client = None - -from galaxy.web_stack import register_postfork_function - - -RAVEN_IMPORT_MESSAGE = ('The Python raven package is required to use this ' - 'feature, please install it') - - -class Sentry: - """ - A WSGI middleware which will attempt to capture any - uncaught exceptions and send them to Sentry. - """ - - def __init__(self, application, dsn, sloreq=0): - assert Client is not None, RAVEN_IMPORT_MESSAGE - self.application = application - self.client = None - self.sloreq_threshold = sloreq - - def postfork_sentry_client(): - self.client = Client(dsn) - - register_postfork_function(postfork_sentry_client) - - def __call__(self, environ, start_response): - try: - start_time = time.time() - iterable = self.application(environ, start_response) - dt = (time.time() - start_time) - if self.sloreq_threshold and dt > self.sloreq_threshold: - self.handle_slow_request(environ, dt) - except Exception: - self.handle_exception(environ) - raise - - try: - yield from iterable - except Exception: - self.handle_exception(environ) - raise - finally: - # wsgi spec requires iterable to call close if it exists - # see http://blog.dscpl.com.au/2012/10/obligations-for-calling-close-on.html - if iterable and hasattr(iterable, 'close') and callable(iterable.close): - try: - iterable.close() - except Exception: - self.handle_exception(environ) - - def handle_slow_request(self, environ, dt): - headers = dict(get_headers(environ)) - if 'Authorization' in headers: - headers['Authorization'] = 'redacted' - if 'Cookie' in headers: - headers['Cookie'] = 'redacted' - cak = environ.get('controller_action_key', None) or environ.get('PATH_INFO', "NOPATH").strip('/').replace('/', '.') - event_id = self.client.captureMessage( - f"SLOREQ: {cak}", - data={ - 'sentry.interfaces.Http': { - 'method': environ.get('REQUEST_METHOD'), - 'url': get_current_url(environ, strip_querystring=True), - 'query_string': environ.get('QUERY_STRING'), - 'headers': headers, - 'env': dict(get_environ(environ)), - } - }, - extra={ - 'request_id': environ.get('request_id', 'Unknown'), - 'request_duration_millis': dt * 1000 - }, - level="warning", - tags={ - 'type': 'sloreq', - 'action_key': cak - } - - ) - # Galaxy: store event_id in environment so we can show it to the user - environ['sentry_event_id'] = event_id - return event_id - - def handle_exception(self, environ): - headers = dict(get_headers(environ)) - # Authorization header for REMOTE_USER sites consists of a base64() of - # their plaintext password. It is a security issue for this password to - # be exposed to a third party system which may or may not be under - # control of the same administrators as the local Authentication - # system. E.g. university LDAP systems. - if 'Authorization' in headers: - # Redact so the administrator knows that a value is indeed present. - headers['Authorization'] = 'redacted' - # Passing cookies allows for impersonation of users (depending on - # remote service) and can be considered a security risk as well. For - # multiple services running alongside Galaxy on the same host, this - # could allow a sentry user with access to logs to impersonate a user - # on another service. In the case of services like Jupyter, this can be - # a serious concern as that would allow for terminal access. Furthermore, - # very little debugging information can be gained as a result of having - # access to all of the users cookies (including Galaxy cookies) - if 'Cookie' in headers: - headers['Cookie'] = 'redacted' - event_id = self.client.captureException( - data={ - 'sentry.interfaces.Http': { - 'method': environ.get('REQUEST_METHOD'), - 'url': get_current_url(environ, strip_querystring=True), - 'query_string': environ.get('QUERY_STRING'), - # TODO - # 'data': environ.get('wsgi.input'), - 'headers': headers, - 'env': dict(get_environ(environ)), - } - }, - # Galaxy: add request id from environment if available - extra={ - 'request_id': environ.get('request_id', 'Unknown') - } - ) - # Galaxy: store event_id in environment so we can show it to the user - environ['sentry_event_id'] = event_id - - return event_id diff --git a/lib/galaxy/webapps/galaxy/buildapp.py b/lib/galaxy/webapps/galaxy/buildapp.py index 827a9a5cdbd..b3bf85b883c 100644 --- a/lib/galaxy/webapps/galaxy/buildapp.py +++ b/lib/galaxy/webapps/galaxy/buildapp.py @@ -1369,10 +1369,9 @@ def wrap_in_middleware(app, global_conf, application_stack, **local_conf): # If sentry logging is enabled, log here before propogating up to # the error middleware sentry_dsn = conf.get('sentry_dsn', None) - sentry_sloreq = float(conf.get('sentry_sloreq_threshold', 0)) if sentry_dsn: - from galaxy.web.framework.middleware.sentry import Sentry - app = wrap_if_allowed(app, stack, Sentry, args=(sentry_dsn, sentry_sloreq)) + from sentry_sdk.integrations.wsgi import SentryWsgiMiddleware + app = wrap_if_allowed(app, stack, SentryWsgiMiddleware) # Various debug middleware that can only be turned on if the debug # flag is set, either because they are insecure or greatly hurt # performance diff --git a/lib/galaxy/webapps/galaxy/config_schema.yml b/lib/galaxy/webapps/galaxy/config_schema.yml index 4131099732a..ff602340cc8 100644 --- a/lib/galaxy/webapps/galaxy/config_schema.yml +++ b/lib/galaxy/webapps/galaxy/config_schema.yml @@ -2029,16 +2029,13 @@ mapping: indicated sentry instance. This connection string is available in your sentry instance under -> Settings -> API Keys. - sentry_sloreq_threshold: - type: float - default: 0.0 + sentry_event_level: + type: str + default: ERROR required: false desc: | - Sentry slow request logging. Requests slower than the threshold - indicated below will be sent as events to the configured Sentry - server (above, sentry_dsn). A value of '0' is disabled. For - example, you would set this to .005 to log all queries taking longer - than 5 milliseconds. + Determines the minimum log level that will be sent as an event to Sentry. + Possible values are DEBUG, INFO, WARNING, ERROR or CRITICAL. statsd_host: type: str diff --git a/lib/galaxy/webapps/galaxy/fast_app.py b/lib/galaxy/webapps/galaxy/fast_app.py index f27d236d327..27051feccc6 100644 --- a/lib/galaxy/webapps/galaxy/fast_app.py +++ b/lib/galaxy/webapps/galaxy/fast_app.py @@ -80,6 +80,11 @@ def add_galaxy_middleware(app: FastAPI, gx_app): nginx_x_accel_redirect_base = gx_app.config.nginx_x_accel_redirect_base apache_xsendfile = gx_app.config.apache_xsendfile + + if gx_app.config.sentry_dsn: + from sentry_sdk.integrations.asgi import SentryAsgiMiddleware + app.add_middleware(SentryAsgiMiddleware) + if nginx_x_accel_redirect_base or apache_xsendfile: @app.middleware("http")