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

internal: add a marker for the first span event for each exception #899

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
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
12 changes: 12 additions & 0 deletions logfire/_internal/tracer.py
Original file line number Diff line number Diff line change
@@ -384,6 +384,18 @@ def record_exception(
stacktrace = ''.join(traceback.format_exception(type(exception), exception, exception.__traceback__))
attributes[SpanAttributes.EXCEPTION_STACKTRACE] = stacktrace

if not getattr(exception, '_logfire_recorded', False):
# This exception has already been recorded, mark it as such in attributes so that
# our backend can replicate the behavior of exceptions as OTEL logs, see
# https://github.com/open-telemetry/opentelemetry-specification/pull/4430#issue-2876146448
# and linked issues.
attributes['logfire.exception_first_recorded'] = True
Copy link
Member Author

Choose a reason for hiding this comment

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

@alexmojaki the idea is that we only explode exception span events if this attribute is set

try:
setattr(exception, '_logfire_recorded', True)
except Exception: # pragma: no cover
# immutable exception objects, etc.
pass

span.record_exception(exception, attributes=attributes, timestamp=timestamp, escaped=escaped)


1 change: 1 addition & 0 deletions tests/otel_integrations/test_django.py
Original file line number Diff line number Diff line change
@@ -195,6 +195,7 @@ def test_error_route(client: Client, exporter: TestExporter):
'exception.message': 'bad request',
'exception.stacktrace': 'django.core.exceptions.BadRequest: bad request',
'exception.escaped': 'False',
'logfire.exception_first_recorded': True,
},
}
],
2 changes: 2 additions & 0 deletions tests/otel_integrations/test_fastapi.py
Original file line number Diff line number Diff line change
@@ -1236,6 +1236,7 @@ def test_fastapi_unhandled_exception(client: TestClient, exporter: TestExporter)
'exception.message': 'test exception',
'exception.stacktrace': 'ValueError: test exception',
'exception.escaped': 'True',
'logfire.exception_first_recorded': True,
},
}
],
@@ -1344,6 +1345,7 @@ def test_fastapi_handled_exception(client: TestClient, exporter: TestExporter) -
'exception.message': '[]',
'exception.stacktrace': 'fastapi.exceptions.RequestValidationError: []',
'exception.escaped': 'True',
'logfire.exception_first_recorded': True,
},
}
],
1 change: 1 addition & 0 deletions tests/otel_integrations/test_starlette.py
Original file line number Diff line number Diff line change
@@ -224,6 +224,7 @@ def test_scrubbing(client: TestClient, exporter: TestExporter) -> None:
'exception.message': 'test exception',
'exception.stacktrace': 'ValueError: test exception',
'exception.escaped': 'False',
'logfire.exception_first_recorded': True,
},
}
],
1 change: 1 addition & 0 deletions tests/test_auto_trace.py
Original file line number Diff line number Diff line change
@@ -130,6 +130,7 @@ def test_auto_trace_sample(exporter: TestExporter) -> None:
'exception.message': 'list index out of range',
'exception.stacktrace': 'IndexError: list index out of range',
'exception.escaped': 'True',
'logfire.exception_first_recorded': True,
},
}
],
1 change: 1 addition & 0 deletions tests/test_console_exporter.py
Original file line number Diff line number Diff line change
@@ -728,6 +728,7 @@ def test_exception(exporter: TestExporter) -> None:
'exception.message': 'division by zero',
'exception.stacktrace': 'ZeroDivisionError: division by zero',
'exception.escaped': 'False',
'logfire.exception_first_recorded': True,
},
}
],
23 changes: 23 additions & 0 deletions tests/test_logfire.py
Original file line number Diff line number Diff line change
@@ -1194,6 +1194,7 @@ def run(a: str) -> Model:
}
]
),
'logfire.exception_first_recorded': True,
},
}
],
@@ -1265,6 +1266,7 @@ def run(a: str) -> None:
}
]
),
'logfire.exception_first_recorded': True,
},
}
],
@@ -2108,6 +2110,7 @@ def test_exc_info(exporter: TestExporter):

for span_dict in span_dicts[2:4]:
[event] = span_dict['events']
event['attributes'].pop('logfire.exception_first_recorded', None)
assert event['attributes'] == {
'exception.type': 'TypeError',
'exception.message': 'other error',
@@ -2117,6 +2120,7 @@ def test_exc_info(exporter: TestExporter):

for span_dict in span_dicts[4:]:
[event] = span_dict['events']
event['attributes'].pop('logfire.exception_first_recorded', None)
assert event['attributes'] == {
'exception.type': 'ValueError',
'exception.message': 'an error',
@@ -3242,3 +3246,22 @@ def test_default_id_generator(exporter: TestExporter) -> None:
export['attributes']['i'] for export in sorted(exported, key=lambda span: span['start_time'])
]
assert sorted_by_trace_id == sorted_by_start_timestamp


def test_exception_duplication_marker_attribute(exporter: TestExporter) -> None:
"""Tests that `logfire.exception_first_recorded` is set on a span event the first time we record an exception
and not on subsequent spans with the same exception.
"""

with pytest.raises(RuntimeError):
with logfire.span('outer'):
with logfire.span('inner'):
raise RuntimeError('error')

spans = exporter.exported_spans_as_dict()
assert len(spans) == 2

outer_span_event = next(s['events'][0] for s in spans if s['name'] == 'outer')
inner_span_event = next(s['events'][0] for s in spans if s['name'] == 'inner')
assert inner_span_event['attributes']['logfire.exception_first_recorded'] is True
assert 'logfire.exception_first_recorded' not in outer_span_event['attributes']
1 change: 1 addition & 0 deletions tests/test_loguru.py
Original file line number Diff line number Diff line change
@@ -94,6 +94,7 @@ def test_loguru(exporter: TestExporter) -> None:
'exception.message': 'This is a test exception',
'exception.stacktrace': 'ValueError: This is a test exception',
'exception.escaped': 'False',
'logfire.exception_first_recorded': True,
},
}
],
3 changes: 3 additions & 0 deletions tests/test_pydantic_plugin.py
Original file line number Diff line number Diff line change
@@ -895,6 +895,7 @@ def validate_x(cls, v: Any) -> Any:
'exception.message': 'My error',
'exception.stacktrace': 'TypeError: My error',
'exception.escaped': 'True',
'logfire.exception_first_recorded': True,
},
}
],
@@ -943,6 +944,7 @@ def validate_x(cls, v: Any) -> Any:
'exception.message': 'My error',
'exception.stacktrace': 'TypeError: My error',
'exception.escaped': 'False',
'logfire.exception_first_recorded': True,
},
}
],
@@ -1054,6 +1056,7 @@ def validate_x(cls, v: Any) -> Any:
'exception.message': 'My error',
'exception.stacktrace': 'TypeError: My error',
'exception.escaped': 'True',
'logfire.exception_first_recorded': True,
},
}
],
1 change: 1 addition & 0 deletions tests/test_secret_scrubbing.py
Original file line number Diff line number Diff line change
@@ -175,6 +175,7 @@ def get_password():
'exception.message': 'Password: hunter2',
'exception.stacktrace': 'wrong and secret',
'exception.escaped': 'False',
'logfire.exception_first_recorded': True,
},
},
{
1 change: 1 addition & 0 deletions tests/test_structlog.py
Original file line number Diff line number Diff line change
@@ -99,6 +99,7 @@ def test_structlog(exporter: TestExporter, logger: Logger) -> None:
'exception.message': 'division by zero',
'exception.stacktrace': 'ZeroDivisionError: division by zero',
'exception.escaped': 'False',
'logfire.exception_first_recorded': True,
},
}
],
1 change: 1 addition & 0 deletions tests/test_testing.py
Original file line number Diff line number Diff line change
@@ -73,6 +73,7 @@ def test_capfire_fixture(capfire: CaptureLogfire) -> None:
'exception.message': 'an exception!',
'exception.stacktrace': 'Exception: an exception!',
'exception.escaped': 'True',
'logfire.exception_first_recorded': True,
},
}
],
Loading