feat(perf): Add detection for render-blocking asset performance issues #37826
Conversation
Tag transactions that have slow asset load spans before a slow FCP as having render-blocking assets. The thresholds are configurable, but currently we're looking for transactions with an FCP between 2s and 10s, where an asset load takes up at least 25% of that time. The thresholds will be tuned before we start generating actual Performance Issues from this data - tagging the transactions will let us see what we're detecting it and validate/tune it before it becomes visible to users. This detector's use of event properties is a little awkward given the current `PerformanceDetector` interface, but I thought it would be better to get the rest of our planned detectors in before we refactor too much. Fixes PERF-1677
github-actions
bot
added
the
Scope: Backend
| "fcp_maximum_threshold": 10000.0, # ms | ||
| "fcp_ratio_threshold": 0.25, | ||
| "allowed_span_ops": ["resource.link", "resource.script"], | ||
| }, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah we can address settings after all the other detectors are done
| # Only concern ourselves with transactions where the FCP is within the | ||
| # range we care about. | ||
| fcp_hash = event.get("measurements", {}).get("fcp", {}) | ||
| if "value" in fcp_hash and ("unit" not in fcp_hash or fcp_hash["unit"] == "millisecond"): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| # Early return for all future span visits. | ||
| self.fcp = None |
There was a problem hiding this comment.
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.
| @@ -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" | |||
There was a problem hiding this comment.
@k-fish Your comment above mentions tag key length limits - do you know what that limit is offhand? This name is probably too long 😬
There was a problem hiding this comment.
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 🤷 )
There was a problem hiding this comment.
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.
Tag transactions that have slow asset load spans before a slow FCP as having render-blocking assets. The thresholds are configurable, but currently we're looking for transactions with an FCP between 2s and 10s, where an asset load takes up at least 25% of that time.
The thresholds will be tuned before we start generating actual Performance Issues from this data - tagging the transactions will let us see what we're detecting it and validate/tune it before it becomes visible to users.
This detector's use of event properties is a little awkward given the current
PerformanceDetectorinterface, but I thought it would be better to get the rest of our planned detectors in before we refactor too much.Fixes PERF-1677