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

Serialize span attrs properly #3668

Merged
merged 6 commits into from
Oct 17, 2024
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
3 changes: 2 additions & 1 deletion sentry_sdk/integrations/asyncpg.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from sentry_sdk.tracing import Span
from sentry_sdk.tracing_utils import add_query_source, record_sql_queries
from sentry_sdk.utils import (
_serialize_span_attribute,
ensure_integration_enabled,
parse_version,
capture_internal_exceptions,
Expand Down Expand Up @@ -147,7 +148,7 @@ def _inner(*args: Any, **kwargs: Any) -> T: # noqa: N807
) as span:
_set_db_data(span, args[0])
res = f(*args, **kwargs)
span.set_attribute("db.cursor", str(res))
span.set_attribute("db.cursor", _serialize_span_attribute(res))

return res

Expand Down
12 changes: 8 additions & 4 deletions sentry_sdk/integrations/clickhouse_driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
from sentry_sdk.integrations import Integration, DidNotEnable
from sentry_sdk.tracing import Span
from sentry_sdk.scope import should_send_default_pii
from sentry_sdk.utils import capture_internal_exceptions, ensure_integration_enabled
from sentry_sdk.utils import (
_serialize_span_attribute,
capture_internal_exceptions,
ensure_integration_enabled,
)

from typing import TYPE_CHECKING, TypeVar

Expand Down Expand Up @@ -99,7 +103,7 @@ def _inner(*args: P.args, **kwargs: P.kwargs) -> T:

if params and should_send_default_pii():
connection._sentry_db_params = params
span.set_attribute("db.params", str(params))
span.set_attribute("db.params", _serialize_span_attribute(params))

# run the original code
ret = f(*args, **kwargs)
Expand All @@ -117,7 +121,7 @@ def _inner_end(*args: P.args, **kwargs: P.kwargs) -> T:

if span is not None:
if res is not None and should_send_default_pii():
span.set_attribute("db.result", str(res))
span.set_attribute("db.result", _serialize_span_attribute(res))

with capture_internal_exceptions():
query = span.get_attribute("db.query.text")
Expand Down Expand Up @@ -159,7 +163,7 @@ def _inner_send_data(*args: P.args, **kwargs: P.kwargs) -> T:
getattr(instance.connection, "_sentry_db_params", None) or []
)
db_params.extend(data)
span.set_attribute("db.params", str(db_params))
span.set_attribute("db.params", _serialize_span_attribute(db_params))
try:
del instance.connection._sentry_db_params
except AttributeError:
Expand Down
9 changes: 5 additions & 4 deletions sentry_sdk/tracing_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
import os
import re
import sys
import uuid
from collections.abc import Mapping
from datetime import datetime, timedelta, timezone
from functools import wraps
from urllib.parse import quote, unquote
import uuid

import sentry_sdk
from sentry_sdk.consts import OP, SPANDATA
Expand All @@ -23,6 +23,7 @@
_is_external_source,
_is_in_project_root,
_module_in_list,
_serialize_span_attribute,
)

from typing import TYPE_CHECKING
Expand Down Expand Up @@ -133,13 +134,13 @@ def record_sql_queries(

data = {}
if params_list is not None:
data["db.params"] = str(params_list)
data["db.params"] = _serialize_span_attribute(params_list)
if paramstyle is not None:
data["db.paramstyle"] = str(paramstyle)
data["db.paramstyle"] = _serialize_span_attribute(paramstyle)
if executemany:
data["db.executemany"] = True
if record_cursor_repr and cursor is not None:
data["db.cursor"] = str(cursor)
data["db.cursor"] = _serialize_span_attribute(cursor)

with capture_internal_exceptions():
sentry_sdk.add_breadcrumb(message=query, category="query", data=data)
Expand Down
26 changes: 26 additions & 0 deletions sentry_sdk/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
)

from gevent.hub import Hub as GeventHub
from opentelemetry.util.types import AttributeValue

from sentry_sdk._types import Event, ExcInfo

Expand Down Expand Up @@ -1831,3 +1832,28 @@ def get_current_thread_meta(thread=None):

# we've tried everything, time to give up
return None, None


def _serialize_span_attribute(value):
# type: (Any) -> Optional[AttributeValue]
"""Serialize an object so that it's OTel-compatible and displays nicely in Sentry."""
# check for allowed primitives
if isinstance(value, (int, str, float, bool)):
return value

# lists are allowed too, as long as they don't mix types
if isinstance(value, (list, tuple)):
for type_ in (int, str, float, bool):
if all(isinstance(item, type_) for item in value):
return list(value)

# if this is anything else, just try to coerce to string
# we prefer json.dumps since this makes things like dictionaries display
# nicely in the UI
try:
return json.dumps(value)
except TypeError:
try:
return str(value)
except Exception:
return None
48 changes: 23 additions & 25 deletions tests/integrations/clickhouse_driver/test_clickhouse_driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
if clickhouse_driver.VERSION < (0, 2, 6):
EXPECT_PARAMS_IN_SELECT = False

PARAMS_SERIALIZER = str


def test_clickhouse_client_breadcrumbs(sentry_init, capture_events) -> None:
sentry_init(
Expand Down Expand Up @@ -144,7 +142,7 @@ def test_clickhouse_client_breadcrumbs_with_pii(sentry_init, capture_events) ->
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.result": str([]),
"db.result": [],
},
"message": "DROP TABLE IF EXISTS test",
"type": "default",
Expand All @@ -157,7 +155,7 @@ def test_clickhouse_client_breadcrumbs_with_pii(sentry_init, capture_events) ->
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.result": str([]),
"db.result": [],
},
"message": "CREATE TABLE test (x Int32) ENGINE = Memory",
"type": "default",
Expand All @@ -170,7 +168,7 @@ def test_clickhouse_client_breadcrumbs_with_pii(sentry_init, capture_events) ->
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.params": PARAMS_SERIALIZER([{"x": 100}]),
"db.params": '[{"x": 100}]',
},
"message": "INSERT INTO test (x) VALUES",
"type": "default",
Expand All @@ -183,7 +181,7 @@ def test_clickhouse_client_breadcrumbs_with_pii(sentry_init, capture_events) ->
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.params": PARAMS_SERIALIZER([[170], [200]]),
"db.params": "[[170], [200]]",
},
"message": "INSERT INTO test (x) VALUES",
"type": "default",
Expand All @@ -196,8 +194,8 @@ def test_clickhouse_client_breadcrumbs_with_pii(sentry_init, capture_events) ->
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.result": str([[370]]),
"db.params": PARAMS_SERIALIZER({"minv": 150}),
"db.result": "[[370]]",
"db.params": '{"minv": 150}',
},
"message": "SELECT sum(x) FROM test WHERE x > 150",
"type": "default",
Expand Down Expand Up @@ -396,7 +394,7 @@ def test_clickhouse_client_spans_with_pii(
"server.address": "localhost",
"server.port": 9000,
"db.query.text": "DROP TABLE IF EXISTS test",
"db.result": str([]),
"db.result": [],
},
"trace_id": transaction_trace_id,
"parent_span_id": transaction_span_id,
Expand All @@ -413,7 +411,7 @@ def test_clickhouse_client_spans_with_pii(
"db.name": "",
"db.user": "default",
"db.query.text": "CREATE TABLE test (x Int32) ENGINE = Memory",
"db.result": str([]),
"db.result": [],
"server.address": "localhost",
"server.port": 9000,
},
Expand All @@ -432,7 +430,7 @@ def test_clickhouse_client_spans_with_pii(
"db.name": "",
"db.user": "default",
"db.query.text": "INSERT INTO test (x) VALUES",
"db.params": PARAMS_SERIALIZER([{"x": 100}]),
"db.params": '[{"x": 100}]',
"server.address": "localhost",
"server.port": 9000,
},
Expand Down Expand Up @@ -468,9 +466,9 @@ def test_clickhouse_client_spans_with_pii(
"db.system": "clickhouse",
"db.name": "",
"db.user": "default",
"db.params": PARAMS_SERIALIZER({"minv": 150}),
"db.params": '{"minv": 150}',
"db.query.text": "SELECT sum(x) FROM test WHERE x > 150",
"db.result": str([(370,)]),
"db.result": "[[370]]",
"server.address": "localhost",
"server.port": 9000,
},
Expand Down Expand Up @@ -622,7 +620,7 @@ def test_clickhouse_dbapi_breadcrumbs_with_pii(sentry_init, capture_events) -> N
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.result": str([[], []]),
"db.result": "[[], []]",
},
"message": "DROP TABLE IF EXISTS test",
"type": "default",
Expand All @@ -635,7 +633,7 @@ def test_clickhouse_dbapi_breadcrumbs_with_pii(sentry_init, capture_events) -> N
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.result": str([[], []]),
"db.result": "[[], []]",
},
"message": "CREATE TABLE test (x Int32) ENGINE = Memory",
"type": "default",
Expand All @@ -648,7 +646,7 @@ def test_clickhouse_dbapi_breadcrumbs_with_pii(sentry_init, capture_events) -> N
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.params": PARAMS_SERIALIZER([{"x": 100}]),
"db.params": '[{"x": 100}]',
},
"message": "INSERT INTO test (x) VALUES",
"type": "default",
Expand All @@ -661,7 +659,7 @@ def test_clickhouse_dbapi_breadcrumbs_with_pii(sentry_init, capture_events) -> N
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.params": PARAMS_SERIALIZER([[170], [200]]),
"db.params": "[[170], [200]]",
},
"message": "INSERT INTO test (x) VALUES",
"type": "default",
Expand All @@ -674,8 +672,8 @@ def test_clickhouse_dbapi_breadcrumbs_with_pii(sentry_init, capture_events) -> N
"db.user": "default",
"server.address": "localhost",
"server.port": 9000,
"db.params": PARAMS_SERIALIZER({"minv": 150}),
"db.result": str([[["370"]], [["'sum(x)'", "'Int64'"]]]),
"db.params": '{"minv": 150}',
"db.result": '[[["370"]], [["\'sum(x)\'", "\'Int64\'"]]]',
},
"message": "SELECT sum(x) FROM test WHERE x > 150",
"type": "default",
Expand Down Expand Up @@ -869,7 +867,7 @@ def test_clickhouse_dbapi_spans_with_pii(
"db.name": "",
"db.user": "default",
"db.query.text": "DROP TABLE IF EXISTS test",
"db.result": str(([], [])),
"db.result": "[[], []]",
"server.address": "localhost",
"server.port": 9000,
},
Expand All @@ -888,7 +886,7 @@ def test_clickhouse_dbapi_spans_with_pii(
"db.name": "",
"db.user": "default",
"db.query.text": "CREATE TABLE test (x Int32) ENGINE = Memory",
"db.result": str(([], [])),
"db.result": "[[], []]",
"server.address": "localhost",
"server.port": 9000,
},
Expand All @@ -907,7 +905,7 @@ def test_clickhouse_dbapi_spans_with_pii(
"db.name": "",
"db.user": "default",
"db.query.text": "INSERT INTO test (x) VALUES",
"db.params": PARAMS_SERIALIZER([{"x": 100}]),
"db.params": '[{"x": 100}]',
"server.address": "localhost",
"server.port": 9000,
},
Expand All @@ -926,7 +924,7 @@ def test_clickhouse_dbapi_spans_with_pii(
"db.name": "",
"db.user": "default",
"db.query.text": "INSERT INTO test (x) VALUES",
"db.params": PARAMS_SERIALIZER([[170], [200]]),
"db.params": "[[170], [200]]",
"server.address": "localhost",
"server.port": 9000,
},
Expand All @@ -945,8 +943,8 @@ def test_clickhouse_dbapi_spans_with_pii(
"db.name": "",
"db.user": "default",
"db.query.text": "SELECT sum(x) FROM test WHERE x > 150",
"db.params": PARAMS_SERIALIZER({"minv": 150}),
"db.result": str(([(370,)], [("sum(x)", "Int64")])),
"db.params": '{"minv": 150}',
"db.result": '[[[370]], [["sum(x)", "Int64"]]]',
"server.address": "localhost",
"server.port": 9000,
},
Expand Down
8 changes: 4 additions & 4 deletions tests/integrations/django/test_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ def test_sql_queries(sentry_init, capture_events, with_integration):
crumb = event["breadcrumbs"]["values"][-1]

assert crumb["message"] == "SELECT count(*) FROM people_person WHERE foo = %s"
assert crumb["data"]["db.params"] == "[123]"
assert crumb["data"]["db.params"] == [123]


@pytest.mark.forked
Expand Down Expand Up @@ -411,7 +411,7 @@ def test_sql_dict_query_params(sentry_init, capture_events):
assert crumb["message"] == (
"SELECT count(*) FROM people_person WHERE foo = %(my_foo)s"
)
assert crumb["data"]["db.params"] == str({"my_foo": 10})
assert crumb["data"]["db.params"] == '{"my_foo": 10}'


@pytest.mark.forked
Expand Down Expand Up @@ -473,7 +473,7 @@ def test_sql_psycopg2_string_composition(sentry_init, capture_events, query):
(event,) = events
crumb = event["breadcrumbs"]["values"][-1]
assert crumb["message"] == ('SELECT %(my_param)s FROM "foobar"')
assert crumb["data"]["db.params"] == str({"my_param": 10})
assert crumb["data"]["db.params"] == '{"my_param": 10}'


@pytest.mark.forked
Expand Down Expand Up @@ -526,7 +526,7 @@ def test_sql_psycopg2_placeholders(sentry_init, capture_events):
{
"category": "query",
"data": {
"db.params": str({"first_var": "fizz", "second_var": "not a date"}),
"db.params": '{"first_var": "fizz", "second_var": "not a date"}',
"db.paramstyle": "format",
},
"message": 'insert into my_test_table ("foo", "bar") values (%(first_var)s, '
Expand Down
32 changes: 32 additions & 0 deletions tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
_get_installed_modules,
_generate_installed_modules,
ensure_integration_enabled,
_serialize_span_attribute,
)


Expand Down Expand Up @@ -901,3 +902,34 @@ def test_format_timestamp_naive():
# Ensure that some timestamp is returned, without error. We currently treat these as local time, but this is an
# implementation detail which we should not assert here.
assert re.fullmatch(timestamp_regex, format_timestamp(datetime_object))


class NoStr:
def __str__(self):
1 / 0


@pytest.mark.parametrize(
("value", "result"),
(
("meow", "meow"),
(1, 1),
(47.0, 47.0),
(True, True),
(["meow", "bark"], ["meow", "bark"]),
([True, False], [True, False]),
([1, 2, 3], [1, 2, 3]),
([46.5, 47.0, 47.5], [46.5, 47.0, 47.5]),
(["meow", 47], '["meow", 47]'), # mixed types not allowed in a list
(None, "null"),
(
{"cat": "meow", "dog": ["bark", "woof"]},
'{"cat": "meow", "dog": ["bark", "woof"]}',
),
(datetime(2024, 1, 1), "2024-01-01 00:00:00"),
(("meow", "purr"), ["meow", "purr"]),
(NoStr(), None),
),
)
def test_serialize_span_attribute(value, result):
assert _serialize_span_attribute(value) == result
Loading