Skip to content
Closed
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
67 changes: 67 additions & 0 deletions src/sentry/tasks/performance_detection.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class DetectorType(Enum):
SLOW_SPAN = "slow_span"
DUPLICATE_SPANS = "duplicate_spans"
SEQUENTIAL_SLOW_SPANS = "sequential_slow_spans"
UNCOMPRESSED_ASSET_SPAN = "uncompressed_asset"


# Facade in front of performance detection to limit impact of detection on our events ingestion
Expand Down Expand Up @@ -55,6 +56,10 @@ def get_detection_settings():
"duration_threshold": 500.0, # ms
"allowed_span_ops": ["db", "http"],
},
DetectorType.UNCOMPRESSED_ASSET_SPAN: {
"size_threshold_bytes": 500 * 1024,
"allowed_span_ops": ["resource.css", "resource.script"],
Comment on lines +60 to +61
Copy link
Member

Choose a reason for hiding this comment

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

I think it's possible we'll also want some timing threshold on top of this in practice since 500kb might be fine depending on your use case or user conditions.

},
}


Expand All @@ -67,6 +72,7 @@ def _detect_performance_issue(data: Event, sdk_span: Any):
DetectorType.DUPLICATE_SPANS: DuplicateSpanDetector(detection_settings),
DetectorType.SLOW_SPAN: SlowSpanDetector(detection_settings),
DetectorType.SEQUENTIAL_SLOW_SPANS: SequentialSlowSpanDetector(detection_settings),
DetectorType.UNCOMPRESSED_ASSET_SPAN: UncompressedAssetSpanDetector(detection_settings),
}

for span in spans:
Expand Down Expand Up @@ -116,13 +122,26 @@ def _detect_performance_issue(data: Event, sdk_span: Any):
len(sequential_performance_fingerprints),
)

uncompressed_asset_performance_issues = detectors[
DetectorType.UNCOMPRESSED_ASSET_SPAN
].stored_issues
uncompressed_asset_fingerprints = list(uncompressed_asset_performance_issues.keys())
if uncompressed_asset_fingerprints:
first_uncompressed_asset_span = uncompressed_asset_performance_issues[
uncompressed_asset_fingerprints[0]
]
sdk_span.containing_transaction.set_tag(
"_pi_uncompressed_asset", first_uncompressed_asset_span["span_id"]
)

metrics.incr(
"performance.performance_issue.detected",
instance=str(bool(all_fingerprints)),
tags={
"duplicates": bool(len(duplicate_performance_fingerprints)),
"slow_span": bool(len(slow_performance_fingerprints)),
"sequential": bool(len(sequential_performance_fingerprints)),
"uncompressed_asset": bool(len(uncompressed_asset_fingerprints)),
},
)

Expand Down Expand Up @@ -331,3 +350,51 @@ def visit_span(self, span: Span):
fingerprint
] >= timedelta(milliseconds=duration_threshold):
self.stored_issues[fingerprint] = {"span_id": span_id}


class UncompressedAssetSpanDetector(PerformanceDetector):
"""
Checks for large assets that are affecting load time.
"""

__slots__ = "stored_issues"

settings_key = DetectorType.UNCOMPRESSED_ASSET_SPAN

def init(self):
self.stored_issues = {}

def visit_span(self, span: Span) -> None:
op = span.get("op", None)
if not op:
return

allowed_span_ops = self.settings.get("allowed_span_ops")
if op not in allowed_span_ops:
return

data = span.get("data", None)
transfer_size = data and data.get("Transfer Size", None)
encoded_body_size = data and data.get("Encoded Body Size", None)
decoded_body_size = data and data.get("Decoded Body Size", None)
if not (encoded_body_size and decoded_body_size and transfer_size):
return

# Ignore assets from cache, either directly (nothing transferred) or via
# a 304 Not Modified response (transfer is smaller than asset size).
if transfer_size <= 0 or transfer_size < encoded_body_size:
return

# Ignore assets that are already compressed.
if encoded_body_size != decoded_body_size:
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 not sure they are always equal anyway, I'd maybe verify iirc message payload or body payload mean slightly different things, but I can't remember since it's been a while since I've fixed a compression issue. It's either decoded or transfer that can be larger because of headers, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good callout - this was true in my local testing but I'll double-check the SDK & Performance API docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed again that these are the same in practice locally, and it seems safe based on my reading of MDN (both encodedBodySize and decodedBodySize measure the size of the payload body specifically.)

return

# Ignore assets that aren't big enough to worry about.
size_threshold_bytes = self.settings.get("size_threshold_bytes")
if encoded_body_size < size_threshold_bytes:
return

fingerprint = fingerprint_span_op(span)
span_id = span.get("span_id", None)
if fingerprint and span_id and not self.stored_issues.get(fingerprint, False):
self.stored_issues[fingerprint] = {"span_id": span_id}
4 changes: 4 additions & 0 deletions tests/sentry/spans/grouping/test_strategy.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ def with_hash(self, hash: str) -> "SpanBuilder":
self.hash = hash
return self

def with_data(self, data: Any) -> "SpanBuilder":
self.data = data
return self

def build(self) -> Span:
span = {
"trace_id": self.trace_id,
Expand Down
76 changes: 76 additions & 0 deletions tests/sentry/tasks/test_performance_detection.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,3 +253,79 @@ def test_calls_detect_sequential(self):
),
]
)

def test_calls_detect_uncompressed_assets(self):
def asset_event(transfer_size=0, encoded_body_size=0, decoded_body_size=0):
return {
"event_id": "a" * 16,
"spans": [
modify_span_duration(
SpanBuilder()
.with_op("resource.script")
.with_description("https://example.com/app.js")
.with_data(
{
"Transfer Size": transfer_size,
"Encoded Body Size": encoded_body_size,
"Decoded Body Size": decoded_body_size,
}
)
.build(),
100.0,
)
],
}

insignificant_size_bytes = 500
significant_size_bytes = 5 * 1024**2

compressed_asset_event = asset_event(
transfer_size=significant_size_bytes,
encoded_body_size=significant_size_bytes,
decoded_body_size=2 * significant_size_bytes,
)
small_uncompressed_asset_event = asset_event(
transfer_size=insignificant_size_bytes,
encoded_body_size=insignificant_size_bytes,
decoded_body_size=insignificant_size_bytes,
)
cached_uncompressed_asset_event = asset_event(
transfer_size=0,
encoded_body_size=significant_size_bytes,
decoded_body_size=significant_size_bytes,
)
uncompressed_asset_event = asset_event(
transfer_size=significant_size_bytes,
encoded_body_size=significant_size_bytes,
decoded_body_size=significant_size_bytes,
)

sdk_span_mock = Mock()

_detect_performance_issue(compressed_asset_event, sdk_span_mock)
assert sdk_span_mock.containing_transaction.set_tag.call_count == 0

_detect_performance_issue(small_uncompressed_asset_event, sdk_span_mock)
assert sdk_span_mock.containing_transaction.set_tag.call_count == 0

_detect_performance_issue(cached_uncompressed_asset_event, sdk_span_mock)
assert sdk_span_mock.containing_transaction.set_tag.call_count == 0

_detect_performance_issue(uncompressed_asset_event, sdk_span_mock)
assert sdk_span_mock.containing_transaction.set_tag.call_count == 3
sdk_span_mock.containing_transaction.set_tag.assert_has_calls(
[
call(
"_pi_all_issue_count",
1,
),
call(
"_pi_transaction",
"aaaaaaaaaaaaaaaa",
),
call(
"_pi_uncompressed_asset",
"bbbbbbbbbbbbbbbb",
),
]
)