Skip to content

Scoped sentry_sdk.init for per-package logging #610

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

Closed
lsorber opened this issue Jan 26, 2020 · 13 comments
Closed

Scoped sentry_sdk.init for per-package logging #610

lsorber opened this issue Jan 26, 2020 · 13 comments

Comments

@lsorber
Copy link

lsorber commented Jan 26, 2020

It looks like sentry_sdk.init integrations are generally attached in a global way. For example, the LoggingIntegration wraps logging.Logger.callHandlers [1], and ExcepthookIntegration wraps sys.excepthook [2].

Let's say you have multiple Sentry projects (Project A, Project B, Project C) that each correspond to one Python package (Package A, Package B, Package C). All packages could be used independently, so it makes sense to instrument all of them individually with sentry_sdk.init. However, if Package C depends on Package A and B, then it would seem that Project C will receive all error notifications even though some of the errors might actually be caused by Packages A and B. Ideally, I'd like to be able to do a scoped sentry_sdk.init on the three packages so that errors from different packages are routed to their corresponding Sentry project, regardless of their interdependencies.

Is such a use case supported, or are there any plans to support this use case in the future?

Potentially relevant issue: #198.

[1] https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/integrations/logging.py#L72
[2] https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/integrations/excepthook.py#L46

@untitaker
Copy link
Member

untitaker commented Jan 26, 2020

Instead of writing sentry_sdk.init(), write:

_library_client = sentry_sdk.Hub(sentry_sdk.Client(...))

Then you can wrap each codeblock that should go to that different client with:

with _library_client:
    ...

All the integrations work too, just provide them as kwargs to Client as you would do with init. Of course they would still monkeypatch under the hood, but they will always try to use Hub.current which will point to _library_client to figure out which integrations are actually supposed to run and where to send events.

This can also be used to disable the SDK on a per-codeblock level like this:

with sentry_sdk.Hub():
   ...

@lsorber
Copy link
Author

lsorber commented Jan 26, 2020

Thanks for the fast response @untitaker! That seems like it would work nicely.

The only thing that seems a bit inconvenient is that we'd have to wrap every function using the with statement you proposed. Is there an easier way to wrap the entire package's functionality with its _library_client?

@untitaker
Copy link
Member

I mean, you could use a decorator instead, but yes, only this way exists. You will likely also have to write a lot of try-catch blocks and call capture_exception() explicitly a lot.

I think you can build some automatic dispatching based on package name on top of that, but we haven't attempted building such a thing, and I see a few challenges in doing this in a way that is generally useful.

@lsorber
Copy link
Author

lsorber commented Jan 27, 2020

All right, thank you. I think we'll need to investigate a bit more to see how we can make this practical. To make sure I understand:

You will likely also have to write a lot of try-catch blocks and call capture_exception() explicitly a lot.

This would be true even for a global sentry_sdk.init, right? Or why do you mention this?

I think you can build some automatic dispatching based on package name on top of that, but we haven't attempted building such a thing, and I see a few challenges in doing this in a way that is generally useful.

I'm thinking we might try to turn the with statement into a decorator, and perhaps even apply it automatically across the package's functions and classes. Do you see any challenges or road bumps with this approach?

@untitaker
Copy link
Member

This would be true even for a global sentry_sdk.init, right? Or why do you mention this?

I think it will have to happen more often for library instrumentation. For example, an error that bubbles like this will end up being reported with the client of package A (the one that was initialized with init):

# package A
def foo():
    return bar()

# package B
def bar():
    with _library_client:
        1/0

Since the exception will just bubble past the context manager.

Do you see any challenges or road bumps with this approach?

That should be fine.

@untitaker
Copy link
Member

Closing this as the question has been answered.

@lsorber
Copy link
Author

lsorber commented Feb 2, 2020

@untitaker See below for an implementation of the ideas discussed above. By applying log_module_with_sentry() at the bottom of a module, all function definitions in that module (including those in classes) are wrapped with a Sentry context manager that also captures exceptions.

As you had predicted, I found that it is indeed necessary to capture exceptions to prevent these from "bubbling past the context manager". While capturing the exception in the wrapper "solves" the logging of uncaught exceptions, I wonder if other Sentry integrations will work as intended without special treatment?

Any further suggestions on how to improve scoped Sentry logging are very welcome!

import ast
import inspect
from types import ModuleType
from typing import Any, Callable, Dict, List, Optional

import sentry_sdk
import wrapt

# https://docs.sentry.io/error-reporting/configuration/?platform=python#common-options
sentry_client = sentry_sdk.Hub(sentry_sdk.Client(dsn="..."))


@wrapt.decorator
def log_function_with_sentry(wrapped: Callable[..., Any], instance: Any, args: List[Any], kwargs: Dict[str, Any]) -> Any:
    """Attaches Sentry integrations to a function."""
    with sentry_client:
        try:
            return wrapped(*args, **kwargs)
        except Exception as e:
            sentry_sdk.capture_exception(e)
            raise


def log_module_with_sentry(module: Optional[ModuleType] = None) -> None:
    """Attaches Sentry integrations to a module."""
    module = module or inspect.getmodule(inspect.stack()[1][0])
    for node in ast.parse(inspect.getsource(module)).body:  # type: ignore
        if isinstance(node, ast.ClassDef):
            cls = getattr(module, node.name)
            for key, value in cls.__dict__.items():
                if callable(value) or isinstance(value, (classmethod, staticmethod)):
                    setattr(cls, key, log_function_with_sentry(value))
        elif isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)):
            setattr(module, node.name, log_function_with_sentry(getattr(module, node.name)))

@lsorber
Copy link
Author

lsorber commented Feb 3, 2020

Another idea could be to only use the __enter__ portion of the Hub's context manager in the function wrapper, that would obviate the need to explicitly capture exceptions.

The only downside would be that a package's Hub/Client might log a parent package's exceptions if the parent package forgets to wrap it's functionality, but that's probably a better alternative than missing the exception outright.

@untitaker
Copy link
Member

That seems like a fine approach, and I think it's as close to a generic impl as you can get. I still think that this makes too many assumptions about how the library API works (assumptions which might be common sense), so I think libraries should vendor this logic and adapt it to their own usecases. For example this somewhat falls apart with underscore-prefixed types which would normally not constitute an API boundary, or when passing callbacks into a library method/function, or when a library method/function returns another function.

A bug that I would fix is that your wrapper for AsyncFunctionDef should likely await for the return value within the try-except. Otherwise you are only catching exceptions for the timeframe where the future/task/promise is created, not when it is being executed.

@SanketDG
Copy link

Reviving this thread again, since we have a similar requirement!

I have this python script as follows:

import sentry_sdk

first_client = sentry_sdk.init(......)

second_client = sentry_sdk.Hub(sentry_sdk.Client(.........))


with second_client:
    l = {}
    try:
        x = l['error']
    except Exception as e:
        sentry_sdk.capture_exception(e)

As per @untitaker's comment this should emit to the second project right? But irrespective of whether I catch it or not, it always goes to the first client.

Any ideas how to get around this? This was a nice way of wrapping code against a certain project, right now we have to import the first client from some other code and switch it at runtime, which can lead to erroneous code.

@sl0thentr0py
Copy link
Member

@SanketDG I just tried out

import sentry_sdk

DSN1 = "<dsn1>"
DSN2 = "<dsn2"

first_client = sentry_sdk.init(DSN1, debug=True)
second_client = sentry_sdk.Hub(sentry_sdk.Client(DSN2, debug=True))

try:
    1 / 0
except Exception as e:
    sentry_sdk.capture_exception(e)


with second_client:
    l = {}
    try:
        x = l['error']
    except Exception as e:
        sentry_sdk.capture_exception(e)

and I successfully get the errors in sentry in different projects.

@SanketDG
Copy link

@sl0thentr0py

If I remove this part of your code:

try:
    1 / 0
except Exception as e:
    sentry_sdk.capture_exception(e)

It does not seem to work. Any idea why both projects need to generate errors?

Debug output:

 [sentry] DEBUG: Flushing HTTP transport
 [sentry] DEBUG: background worker got flush request
 [sentry] DEBUG: Sending event, type:null level:error event_id:94d1d5c33dd046c081b4395d0abf27c6 project:6382112 host:o1233348.ingest.sentry.io
 [sentry] DEBUG: background worker flushed
 [sentry] DEBUG: Killing HTTP transport
 [sentry] DEBUG: background worker got kill request

@sl0thentr0py
Copy link
Member

@SanketDG sorry for the late reply here, you might have to call flush on the second hub, this works for me. This is because the atexit integration hooks into the main hub so it doesn't get caught here I think.

(also did s/client/hub/ in the sample code to be more precise in terminology)

import sentry_sdk

DSN1 = "<dsn1>"
DSN2 = "<dsn2>"

first_hub = sentry_sdk.init(DSN1, debug=True)
second_hub = sentry_sdk.Hub(sentry_sdk.Client(DSN2, debug=True))

with second_hub:
    l = {}
    try:
        x = l['adasdasdasdas']
    except Exception as e:
        sentry_sdk.capture_exception(e)

second_hub.flush()

In general, some of the integrations logic might not work with such multi-hub setups, please report if something doesn't work and we'll try to figure it out together. These are sort of edge cases and the SDK is definitely not built with such scenarios in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants