Skip to content
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
84 changes: 78 additions & 6 deletions src/sentry/utils/performance_issues/performance_detection.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class DetectorType(Enum):
DUPLICATE_SPANS = "duplicates"
SEQUENTIAL_SLOW_SPANS = "sequential"
LONG_TASK_SPANS = "long_task"
RENDER_BLOCKING_ASSET_SPAN = "render_blocking_assets"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@k-fish Your comment above mentions tag key length limits - do you know what that limit is offhand? This name is probably too long 😬

Copy link
Member

Choose a reason for hiding this comment

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

I already counted when I did the review, it should be fine. (limit is 32, that sentence is 22, we add less than 10 elsewhere, it's close but 🤷 )

Copy link
Member

Choose a reason for hiding this comment

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

Tag keys have a maximum length of 32 characters and can contain only letters (a-zA-Z), numbers (0-9), underscores (_), periods (.), colons (:), and dashes (-).

Tag values have a maximum length of 200 characters and they cannot contain the newline (\n) character.



# Facade in front of performance detection to limit impact of detection on our events ingestion
Expand Down Expand Up @@ -85,6 +86,12 @@ def get_default_detection_settings():
"allowed_span_ops": ["ui.long-task", "ui.sentry.long-task"],
}
],
DetectorType.RENDER_BLOCKING_ASSET_SPAN: {
"fcp_minimum_threshold": 2000.0, # ms
"fcp_maximum_threshold": 10000.0, # ms
"fcp_ratio_threshold": 0.25,
"allowed_span_ops": ["resource.link", "resource.script"],
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making this an array like the others is awkward: the settings aren't op-specific, but even a single-item array with settings_for_span can't work because the FCP min/max thresholds aren't used in the context of an individual span at all. Works fine as a hash read directly, but it's inconsistent with the rest of the settings.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we can address settings after all the other detectors are done

}


Expand All @@ -94,11 +101,14 @@ def _detect_performance_issue(data: Event, sdk_span: Any):

detection_settings = get_default_detection_settings()
detectors = {
DetectorType.DUPLICATE_SPANS: DuplicateSpanDetector(detection_settings),
DetectorType.DUPLICATE_SPANS_HASH: DuplicateSpanHashDetector(detection_settings),
DetectorType.SLOW_SPAN: SlowSpanDetector(detection_settings),
DetectorType.SEQUENTIAL_SLOW_SPANS: SequentialSlowSpanDetector(detection_settings),
DetectorType.LONG_TASK_SPANS: LongTaskSpanDetector(detection_settings),
DetectorType.DUPLICATE_SPANS: DuplicateSpanDetector(detection_settings, data),
DetectorType.DUPLICATE_SPANS_HASH: DuplicateSpanHashDetector(detection_settings, data),
DetectorType.SLOW_SPAN: SlowSpanDetector(detection_settings, data),
DetectorType.SEQUENTIAL_SLOW_SPANS: SequentialSlowSpanDetector(detection_settings, data),
DetectorType.LONG_TASK_SPANS: LongTaskSpanDetector(detection_settings, data),
DetectorType.RENDER_BLOCKING_ASSET_SPAN: RenderBlockingAssetSpanDetector(
detection_settings, data
),
}

for span in spans:
Expand Down Expand Up @@ -143,8 +153,9 @@ class PerformanceDetector(ABC):
Classes of this type have their visit functions called as the event is walked once and will store a performance issue if one is detected.
"""

def __init__(self, settings: Dict[str, Any]):
def __init__(self, settings: Dict[str, Any], event: Event):
self.settings = settings[self.settings_key]
self._event = event
self.init()

@abstractmethod
Expand All @@ -170,6 +181,9 @@ def settings_for_span(self, span: Span):
return op, span_id, op_prefix, span_duration, setting
return None

def event(self) -> Event:
return self._event

@property
@abstractmethod
def settings_key(self) -> DetectorType:
Expand Down Expand Up @@ -411,6 +425,64 @@ def visit_span(self, span: Span):
)


class RenderBlockingAssetSpanDetector(PerformanceDetector):
__slots__ = ("stored_issues", "fcp", "transaction_start")

settings_key = DetectorType.RENDER_BLOCKING_ASSET_SPAN

def init(self):
self.stored_issues = {}
self.transaction_start = timedelta(seconds=self.event().get("transaction_start", 0))
self.fcp = None

# Only concern ourselves with transactions where the FCP is within the
# range we care about.
fcp_hash = self.event().get("measurements", {}).get("fcp", {})
if "value" in fcp_hash and ("unit" not in fcp_hash or fcp_hash["unit"] == "millisecond"):
Copy link
Contributor Author

@mjq-sentry mjq-sentry Aug 15, 2022

Choose a reason for hiding this comment

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

I couldn't find anywhere where we're dealing with different units for measurement values. The browser SDK, which is the only thing generating these resource spans, uses millisecond.

Copy link
Member

Choose a reason for hiding this comment

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

Better to be more restrictive, the output inside our metrics will show us whether it was somehow too restrictive, and we can watch it over time.

fcp = timedelta(milliseconds=fcp_hash.get("value"))
fcp_minimum_threshold = timedelta(
milliseconds=self.settings.get("fcp_minimum_threshold")
)
fcp_maximum_threshold = timedelta(
milliseconds=self.settings.get("fcp_maximum_threshold")
)
if fcp >= fcp_minimum_threshold and fcp < fcp_maximum_threshold:
self.fcp = fcp

def visit_span(self, span: Span):
if not self.fcp:
return

op = span.get("op", None)
allowed_span_ops = self.settings.get("allowed_span_ops")
if op not in allowed_span_ops:
return False

if self._is_blocking_render(span):
span_id = span.get("span_id", None)
fingerprint = fingerprint_span(span)
if span_id and fingerprint:
self.stored_issues[fingerprint] = PerformanceSpanIssue(span_id, op, [span_id])

# If we visit a span that starts after FCP, then we know we've already
# seen all possible render-blocking resource spans.
span_start_timestamp = timedelta(seconds=span.get("start_timestamp", 0))
fcp_timestamp = self.transaction_start + self.fcp
if span_start_timestamp >= fcp_timestamp:
# Early return for all future span visits.
self.fcp = None
Comment on lines +472 to +473
Copy link
Member

Choose a reason for hiding this comment

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

Yeah we can either have visit_span return, or we set it up so visit_span is indirectly called from a base function which checks some base property like PerformanceDetector.isDisabled so each class has more control.


def _is_blocking_render(self, span):
span_end_timestamp = timedelta(seconds=span.get("timestamp", 0))
fcp_timestamp = self.transaction_start + self.fcp
if span_end_timestamp >= fcp_timestamp:
return False

span_duration = get_span_duration(span)
fcp_ratio_threshold = self.settings.get("fcp_ratio_threshold")
return span_duration / self.fcp > fcp_ratio_threshold


# Reports metrics and creates spans for detection
def report_metrics_for_detectors(
event_id: Optional[str], detectors: Dict[str, PerformanceDetector], sdk_span: Any
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,3 +283,71 @@ def test_calls_detect_long_task(self):
),
]
)

def test_calls_detect_render_blocking_asset(self):
render_blocking_asset_event = {
"event_id": "a" * 16,
"measurements": {
"fcp": {
"value": 2500.0,
"unit": "millisecond",
}
},
"spans": [
create_span("resource.script", duration=1000.0),
],
}
non_render_blocking_asset_event = {
"event_id": "a" * 16,
"measurements": {
"fcp": {
"value": 2500.0,
"unit": "millisecond",
}
},
"spans": [
modify_span_start(
create_span("resource.script", duration=1000.0),
2000.0,
),
],
}
short_render_blocking_asset_event = {
"event_id": "a" * 16,
"measurements": {
"fcp": {
"value": 2500.0,
"unit": "millisecond",
}
},
"spans": [
create_span("resource.script", duration=200.0),
],
}

sdk_span_mock = Mock()

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

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

_detect_performance_issue(render_blocking_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_render_blocking_assets",
"bbbbbbbbbbbbbbbb",
),
]
)