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

Centralized way to configure custom instrumentation #1960

Merged
merged 15 commits into from
Mar 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions sentry_sdk/client.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from importlib import import_module
import os
import uuid
import random
Expand All @@ -17,6 +18,7 @@
logger,
)
from sentry_sdk.serializer import serialize
from sentry_sdk.tracing import trace
from sentry_sdk.transport import make_transport
from sentry_sdk.consts import (
DEFAULT_OPTIONS,
Expand All @@ -38,6 +40,7 @@
from typing import Callable
from typing import Dict
from typing import Optional
from typing import Sequence

from sentry_sdk.scope import Scope
from sentry_sdk._types import Event, Hint
Expand Down Expand Up @@ -118,6 +121,14 @@ def _get_options(*args, **kwargs):
return rv


try:
# Python 3.6+
module_not_found_error = ModuleNotFoundError
except Exception:
# Older Python versions
module_not_found_error = ImportError # type: ignore


class _Client(object):
"""The client is internally responsible for capturing the events and
forwarding them to sentry through the configured transport. It takes
Expand All @@ -140,6 +151,52 @@ def __setstate__(self, state):
self.options = state["options"]
self._init_impl()

def _setup_instrumentation(self, functions_to_trace):
# type: (Sequence[Dict[str, str]]) -> None
"""
Instruments the functions given in the list `functions_to_trace` with the `@sentry_sdk.tracing.trace` decorator.
"""
for function in functions_to_trace:
class_name = None
function_qualname = function["qualified_name"]
module_name, function_name = function_qualname.rsplit(".", 1)

try:
# Try to import module and function
# ex: "mymodule.submodule.funcname"

module_obj = import_module(module_name)
function_obj = getattr(module_obj, function_name)
setattr(module_obj, function_name, trace(function_obj))
logger.debug("Enabled tracing for %s", function_qualname)

except module_not_found_error:
try:
# Try to import a class
# ex: "mymodule.submodule.MyClassName.member_function"

module_name, class_name = module_name.rsplit(".", 1)
module_obj = import_module(module_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this import_module is a lot of magic, because

  • you might accidentally run user code that was not supposed to run just because sentry needs to know what's in that namespace
  • is also a potential security issue / injection vector since you're letting users specify arbitrary modules and then actually performing IO on them. This is basically an eval call and we should be very careful of introducing such things in our codebase.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, true. If there is code executed on module load it could be have unintentional behavior.

But this optional and is only active when functions_to_trace is set.

What if we point out this concerns in the documentation, so people know about them when activating this feature?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitsuhiko can I get your opinion on this? Should we avoid putting import_module in the SDK (although it needs to be enabled by default)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m okay with importing from strings. But I can’t say about the feature as such.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying you can not say anything about the feature, or are you saying that you are not okay with the feature @mitsuhiko ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(On another note, @mdtro (Matthew T) said in slack: A note in the documentation is sufficient, imo. The list should be static or loaded from a secure source (no user or untrusted input).)

class_obj = getattr(module_obj, class_name)
function_obj = getattr(class_obj, function_name)
setattr(class_obj, function_name, trace(function_obj))
setattr(module_obj, class_name, class_obj)
logger.debug("Enabled tracing for %s", function_qualname)

except Exception as e:
logger.warning(
"Can not enable tracing for '%s'. (%s) Please check your `functions_to_trace` parameter.",
function_qualname,
e,
)

except Exception as e:
logger.warning(
"Can not enable tracing for '%s'. (%s) Please check your `functions_to_trace` parameter.",
function_qualname,
e,
)

def _init_impl(self):
# type: () -> None
old_debug = _client_init_debug.get(False)
Expand Down Expand Up @@ -184,6 +241,8 @@ def _capture_envelope(envelope):
except ValueError as e:
logger.debug(str(e))

self._setup_instrumentation(self.options.get("functions_to_trace", []))

@property
def dsn(self):
# type: () -> Optional[str]
Expand Down
1 change: 1 addition & 0 deletions sentry_sdk/consts.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ def __init__(
trace_propagation_targets=[ # noqa: B006
MATCH_ALL
], # type: Optional[Sequence[str]]
functions_to_trace=[], # type: Sequence[str] # noqa: B006
event_scrubber=None, # type: Optional[sentry_sdk.scrubber.EventScrubber]
):
# type: (...) -> None
Expand Down
68 changes: 68 additions & 0 deletions tests/test_basics.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import logging
import os
import sys
import time

import pytest

Expand Down Expand Up @@ -618,3 +619,70 @@ def foo(event, hint):
)
def test_get_sdk_name(installed_integrations, expected_name):
assert get_sdk_name(installed_integrations) == expected_name


def _hello_world(word):
return "Hello, {}".format(word)


def test_functions_to_trace(sentry_init, capture_events):
functions_to_trace = [
{"qualified_name": "tests.test_basics._hello_world"},
{"qualified_name": "time.sleep"},
]

sentry_init(
traces_sample_rate=1.0,
functions_to_trace=functions_to_trace,
)

events = capture_events()

with start_transaction(name="something"):
time.sleep(0)

for word in ["World", "You"]:
_hello_world(word)

assert len(events) == 1

(event,) = events

assert len(event["spans"]) == 3
assert event["spans"][0]["description"] == "time.sleep"
assert event["spans"][1]["description"] == "tests.test_basics._hello_world"
assert event["spans"][2]["description"] == "tests.test_basics._hello_world"


class WorldGreeter:
def __init__(self, word):
self.word = word

def greet(self, new_word=None):
return "Hello, {}".format(new_word if new_word else self.word)


def test_functions_to_trace_with_class(sentry_init, capture_events):
functions_to_trace = [
{"qualified_name": "tests.test_basics.WorldGreeter.greet"},
]

sentry_init(
traces_sample_rate=1.0,
functions_to_trace=functions_to_trace,
)

events = capture_events()

with start_transaction(name="something"):
wg = WorldGreeter("World")
wg.greet()
wg.greet("You")

assert len(events) == 1

(event,) = events

assert len(event["spans"]) == 2
assert event["spans"][0]["description"] == "tests.test_basics.WorldGreeter.greet"
assert event["spans"][1]["description"] == "tests.test_basics.WorldGreeter.greet"
1 change: 1 addition & 0 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ deps =
arq: arq>=0.23.0
arq: fakeredis>=2.2.0,<2.8
arq: pytest-asyncio
arq: async-timeout

# Asgi
asgi: pytest-asyncio
Expand Down