Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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
121 changes: 104 additions & 17 deletions sentry_sdk/integrations/chalice.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,23 @@
from functools import wraps

import sentry_sdk
import sentry_sdk.traces
from sentry_sdk.consts import OP
from sentry_sdk.integrations import DidNotEnable, Integration
from sentry_sdk.integrations._wsgi_common import _filter_headers
from sentry_sdk.integrations.aws_lambda import _make_request_event_processor
from sentry_sdk.integrations.cloud_resource_context import (
CLOUD_PLATFORM,
CLOUD_PROVIDER,
)
from sentry_sdk.traces import (
SegmentSource,
SpanStatus,
StreamedSpan,
get_current_span,
)
from sentry_sdk.tracing import TransactionSource
from sentry_sdk.tracing_utils import has_span_streaming_enabled

Check warning on line 21 in sentry_sdk/integrations/chalice.py

View check run for this annotation

@sentry/warden / warden: find-bugs

Missing transaction name on scope in streaming mode when no active segment span exists

In `_get_view_function_response`, when span streaming is enabled (`has_span_streaming_enabled`) but no `StreamedSpan` segment is active (e.g. the Chalice app runs without the AWS Lambda integration, which is the only thing that creates the segment), `segment` stays `None` and the streaming branch never calls `scope.set_transaction_name(...)`. Errors are still captured via `capture_event`, but the resulting events carry no transaction context. The non-streaming `else` branch always sets `scope.set_transaction_name(app.lambda_context.function_name, source=TransactionSource.COMPONENT)`, so for any setup without an active segment this is a behavioral regression where error/message events lose their transaction name.
from sentry_sdk.utils import (
capture_internal_exceptions,
event_from_exception,
Expand Down Expand Up @@ -63,38 +77,89 @@
with sentry_sdk.isolation_scope() as scope:
with capture_internal_exceptions():
configured_time = app.lambda_context.get_remaining_time_in_millis()
scope.set_transaction_name(
app.lambda_context.function_name,
source=TransactionSource.COMPONENT,
)

scope.add_event_processor(
_make_request_event_processor(
app.current_request.to_dict(),
app.lambda_context,
configured_time,
)
)
try:
return view_function(**function_args)
except Exception as exc:
if isinstance(exc, ChaliceViewError):

if has_span_streaming_enabled(client.options):
current_span = get_current_span()
segment = None
if type(current_span) is StreamedSpan:
# A segment already exists (created by the AWS Lambda
# integration), so decorate it with Chalice attributes
# The AWS Lambda integration owns the span lifecycle
# (end + flush), but Chalice converts unhandled view exceptions
# into 500 responses, so the error must be captured here.
aws_context = app.lambda_context
request_dict = app.current_request.to_dict()
headers = request_dict.get("headers", {})

header_attrs: "Dict[str, Any]" = {}
for header, value in _filter_headers(
headers, use_annotated_value=False
).items():
header_attrs[f"http.request.header.{header.lower()}"] = value
Comment on lines +91 to +97

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The Chalice integration can crash with an AttributeError if the incoming request has a "headers" key with a None value, as the code attempts to call .items() on it.
Severity: HIGH

Suggested Fix

Add a check to ensure headers is a dictionary before it's used. A pattern like if not isinstance(headers, dict): headers = {} should be added after retrieving the headers from request_dict, mirroring the safeguard present in the AWS Lambda integration.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: sentry_sdk/integrations/chalice.py#L91-L97

Potential issue: In the Chalice integration, headers are retrieved from the request
dictionary using `request_dict.get("headers", {})`. This does not guard against the case
where the `"headers"` key exists but its value is `None`. If `headers` is `None`, the
subsequent call to `_filter_headers(headers, ...).items()` will raise an
`AttributeError: 'NoneType' object has no attribute 'items'`, causing the application to
crash. This is a realistic scenario as Chalice uses AWS Lambda events, which can have
`None` as the value for headers, a possibility explicitly handled in the AWS Lambda
integration but missed here.


additional_attrs: "Dict[str, Any]" = {}
if "method" in request_dict:
additional_attrs["http.request.method"] = request_dict["method"]

attributes = {
**_get_lambda_span_attributes(aws_context),
**header_attrs,
**additional_attrs,
}

segment = current_span._segment
segment.set_attributes(attributes)

try:
return view_function(**function_args)
except Exception as exc:
if isinstance(exc, ChaliceViewError):
raise
exc_info = sys.exc_info()
if segment:
segment.status = SpanStatus.ERROR.value
sentry_event, hint = event_from_exception(
exc_info,
Comment thread
sentry[bot] marked this conversation as resolved.
client_options=client.options,
mechanism={"type": "chalice", "handled": False},
)
sentry_sdk.capture_event(sentry_event, hint=hint)
if segment is None:
client.flush()
raise
exc_info = sys.exc_info()
event, hint = event_from_exception(
exc_info,
client_options=client.options,
mechanism={"type": "chalice", "handled": False},
else:
scope.set_transaction_name(
app.lambda_context.function_name,
source=TransactionSource.COMPONENT,
)
sentry_sdk.capture_event(event, hint=hint)
client.flush()
raise
try:
return view_function(**function_args)
except Exception as exc:
if isinstance(exc, ChaliceViewError):
raise
exc_info = sys.exc_info()
sentry_event, hint = event_from_exception(
exc_info,
client_options=client.options,
mechanism={"type": "chalice", "handled": False},
)
sentry_sdk.capture_event(sentry_event, hint=hint)
client.flush()
raise

return wrapped_view_function # type: ignore


class ChaliceIntegration(Integration):
identifier = "chalice"
origin = f"auto.function.{identifier}"

@staticmethod
def setup_once() -> None:
Expand Down Expand Up @@ -129,3 +194,25 @@
RestAPIEventHandler._get_view_function_response = sentry_event_response
# for everything else (like events)
chalice.app.EventSourceHandler = EventSourceHandler


def _get_lambda_span_attributes(aws_context: "Any") -> "Dict[str, Any]":
invoked_arn = aws_context.invoked_function_arn
split_invoked_arn = invoked_arn.split(":")
aws_region = split_invoked_arn[3] if len(split_invoked_arn) > 3 else "unknown"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Although AWS sets this value and it's unlikely to be malformed, these extra guards were added to ensure that if it were, we don't crash the user's application


return {
"sentry.op": OP.FUNCTION_AWS,
"sentry.origin": ChaliceIntegration.origin,
"sentry.span.source": SegmentSource.COMPONENT,
"cloud.platform": CLOUD_PLATFORM.AWS_LAMBDA,
"cloud.provider": CLOUD_PROVIDER.AWS,
"faas.name": aws_context.function_name,
"cloud.region": aws_region,
"cloud.resource_id": invoked_arn,
"aws.lambda.invoked_arn": invoked_arn,
"faas.invocation_id": aws_context.aws_request_id,
"faas.version": aws_context.function_version,
"aws.log.group.names": [aws_context.log_group_name],
"aws.log.stream.names": [aws_context.log_stream_name],
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the AWS Lambda integration is active, it's already attached all (?) of these to the segment, right? So this is for the case when the segment is not coming from the AWS Lambda integration, e.g. custom instrumentation?

I'm fine adding this but unless I'm misunderstanding something it's not necessary for feature parity with the non-streaming code path

101 changes: 98 additions & 3 deletions tests/integrations/chalice/test_chalice.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,23 @@
from chalice.local import LambdaContext, LocalGateway
from pytest_chalice.handlers import RequestHandler

import sentry_sdk
from sentry_sdk import capture_message
from sentry_sdk.integrations.chalice import CHALICE_VERSION, ChaliceIntegration
from sentry_sdk.utils import parse_version


def _populate_lambda_context(context):
fn = context.function_name
context.invoked_function_arn = (
f"arn:aws:lambda:us-east-1:123456789012:function:{fn}"
)
context.log_group_name = f"/aws/lambda/{fn}"
context.log_stream_name = "2024/01/01/[$LATEST]abcdef1234567890"
context.aws_request_id = "test-request-id-1234"
return context


def _generate_lambda_context(self):
# Monkeypatch of the function _generate_lambda_context
# from the class LocalGateway
Expand All @@ -19,11 +31,12 @@
timeout = 10 * 1000
else:
timeout = self._config.lambda_timeout * 1000
return LambdaContext(
context = LambdaContext(
function_name=self._config.function_name,
memory_size=self._config.lambda_memory_size,
max_runtime_ms=timeout,
)
return _populate_lambda_context(context)


@pytest.fixture
Expand Down Expand Up @@ -89,8 +102,8 @@
def every_hour(event):
raise Exception("schedule event!")

context = LambdaContext(
*lambda_context_args, max_runtime_ms=10000, time_source=time
context = _populate_lambda_context(
LambdaContext(*lambda_context_args, max_runtime_ms=10000, time_source=time)
)

lambda_event = {
Expand Down Expand Up @@ -160,3 +173,85 @@
(event,) = events
assert event["transaction"] == expected_transaction
assert event["transaction_info"] == {"source": expected_source}


def _make_span_streaming_app(sentry_init):
sentry_init(
integrations=[ChaliceIntegration()],
traces_sample_rate=1.0,
_experiments={"trace_lifecycle": "stream"},
)
app = Chalice(app_name="sentry_chalice")

@app.route("/message")
def hi():
capture_message("hi")
return {"status": "ok"}

@app.route("/boom")
def boom():
raise Exception("boom goes the dynamite!")

LocalGateway._generate_lambda_context = _generate_lambda_context

return app


def test_span_streaming_existing_span(
Comment thread
sentry-warden[bot] marked this conversation as resolved.
sentry_init,
capture_items,
):
"""When a segment already exists (e.g. created by the AWS Lambda
integration), Chalice decorates it instead of creating a duplicate."""
app = _make_span_streaming_app(sentry_init)
client = RequestHandler(app)
items = capture_items("span")

with sentry_sdk.traces.start_span(
name="lambda_segment",
parent_span=None,
attributes={"sentry.origin": "auto.function.aws_lambda"},
):
response = client.get("/message")
assert response.status_code == 200

sentry_sdk.flush()

segment_spans = [s.payload for s in items if s.payload.get("is_segment")]
assert len(segment_spans) == 1
span = segment_spans[0]

attrs = span["attributes"]
assert attrs["sentry.origin"] == "auto.function.chalice"
assert attrs["sentry.op"] == "function.aws"
assert attrs["faas.name"] == "api_handler"
assert span["status"] == "ok"


def test_span_streaming_existing_span_error(
sentry_init,
capture_items,
):
app = _make_span_streaming_app(sentry_init)
client = RequestHandler(app)

Check warning on line 236 in tests/integrations/chalice/test_chalice.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[CR3-M9L] Missing transaction name on scope in streaming mode when no active segment span exists (additional location)

In `_get_view_function_response`, when span streaming is enabled (`has_span_streaming_enabled`) but no `StreamedSpan` segment is active (e.g. the Chalice app runs without the AWS Lambda integration, which is the only thing that creates the segment), `segment` stays `None` and the streaming branch never calls `scope.set_transaction_name(...)`. Errors are still captured via `capture_event`, but the resulting events carry no transaction context. The non-streaming `else` branch always sets `scope.set_transaction_name(app.lambda_context.function_name, source=TransactionSource.COMPONENT)`, so for any setup without an active segment this is a behavioral regression where error/message events lose their transaction name.
items = capture_items("event", "span")

with sentry_sdk.traces.start_span(
name="lambda_segment",
parent_span=None,
attributes={"sentry.origin": "auto.function.aws_lambda"},
):
response = client.get("/boom")
assert response.status_code == 500

sentry_sdk.flush()
Comment thread
sentry-warden[bot] marked this conversation as resolved.

error_items = [i for i in items if i.type == "event"]
assert len(error_items) == 1

segment_spans = [
s.payload for s in items if s.type == "span" and s.payload.get("is_segment")
]
assert len(segment_spans) == 1
assert segment_spans[0]["attributes"]["sentry.origin"] == "auto.function.chalice"
assert segment_spans[0]["status"] == "error"
Loading