Skip to content

Server Timing - Allow description and make duration optional#2379

Merged
westonruter merged 14 commits into
WordPress:trunkfrom
the-hercules:fix/allow-desc-dur-optional-879
Jun 18, 2026
Merged

Server Timing - Allow description and make duration optional#2379
westonruter merged 14 commits into
WordPress:trunkfrom
the-hercules:fix/allow-desc-dur-optional-879

Conversation

@the-hercules

@the-hercules the-hercules commented Feb 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Fixes #879

Relevant technical choices

Enhance Server-Timing API for MDN Specification Compliance

Enhances the Server-Timing API to be fully compliant with the MDN Server-Timing specification by making duration optional and adding support for a desc (description) field.

Changes

class-perflab-server-timing-metric.php

  • Added $description property to Perflab_Server_Timing_Metric
  • Added set_description() and get_description() methods
  • Used native PHP string type hint for set_description() instead of string|mixed with manual type checking
  • Explicitly initialised $description = null for clarity

class-perflab-server-timing.php

  • Updated format_metric_header_value() to conditionally include dur= and desc= parameters
  • Added sanitization for description values to prevent header injection:
    • Escapes \ and "
    • Strips CR/LF control characters
    • Uses a combined addcslashes() call

Tests

  • Added @covers annotations to all new test methods
  • Added tests for all four header output formats:
    • Name-only
    • Duration-only
    • Description-only
    • Combined duration + description
  • Added edge-case tests for special characters in descriptions (", \, \n, \r)
  • Replaced vague @phpstan-param array<string, mixed> with:
    • @param array<string, MetricArguments>
    • Used @phpstan-import-type for accurate type definitions

Supported Header Formats

Format Example Output
Name only Server-Timing: missedCache
Duration only wp-metric;dur=45.23
Description only wp-metric;desc="Cache hit"
Duration + Description wp-metric;dur=45.23;desc="Database queries"

Testing

  • All 137 Server-Timing tests passing on single site and multisite

Fixes

Fixes #879

Use of AI Tools

Github Copilot CLI was used to write tests. Reviewed the code afterwards.
@github-actions

github-actions Bot commented Feb 11, 2026 •
edited
Loading

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: the-hercules <thehercules@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: anandraj346 <anandraj346@git.wordpress.org>
Co-authored-by: dilipom13 <dilip2615@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.
@the-hercules the-hercules changed the title Allow description and make duration optional Feb 11, 2026
@codecov

codecov Bot commented Feb 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.72%. Comparing base (4fdf1e9) to head (abe7684).

Additional details and impacted files 
@@            Coverage Diff             @@
##            trunk    #2379      +/-   ##
==========================================
+ Coverage   69.64%   69.72%   +0.07%     
==========================================
  Files          90       90              
  Lines        7736     7753      +17     
==========================================
+ Hits         5388     5406      +18     
+ Misses       2348     2347       -1
Flag Coverage Δ
multisite 69.72% <100.00%> (+0.07%) ⬆️
single 35.53% <80.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
westonruter
westonruter reviewed Feb 11, 2026
View reviewed changes
Comment thread plugins/performance-lab/includes/server-timing/class-perflab-server-timing-metric.php Outdated
westonruter
westonruter reviewed Feb 11, 2026
View reviewed changes
Comment thread plugins/performance-lab/includes/server-timing/class-perflab-server-timing-metric.php Outdated
westonruter
westonruter reviewed Feb 11, 2026
View reviewed changes
Comment thread plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php Outdated
westonruter
westonruter reviewed Feb 11, 2026
View reviewed changes
Comment thread plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php Outdated
westonruter
westonruter reviewed Feb 11, 2026
View reviewed changes
Comment thread plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php Outdated
westonruter
westonruter reviewed Feb 11, 2026
View reviewed changes
Comment thread plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php Outdated
westonruter
westonruter reviewed Feb 11, 2026
View reviewed changes
Comment thread plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php Outdated
@the-hercules
Use native type hint for set_description(), add @Covers annotations, …
41b677b 
…and improve type definitions

   - Replace string|mixed param with native string type hint in set_description(), removing manual type check
   - Explicitly initialize $description = null
   - Combine addcslashes() calls into single expression
   - Add @Covers annotations to new description test methods
   - Import MetricArguments phpstan type and replace mixed with array<string, MetricArguments> in test param annotations
@the-hercules
chore: Remove redundant test for set_description() requiring a string.
ca2985e
westonruter
westonruter reviewed Feb 26, 2026
View reviewed changes
Comment thread plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php Outdated
@westonruter westonruter requested a review from Copilot February 26, 2026 00:40
@westonruter westonruter added this to the performance-lab 4.0.2 milestone Feb 26, 2026
Copilot AI reviewed Feb 26, 2026
View reviewed changes

Copilot AI left a comment

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.

Pull request overview

Updates the Performance Lab Server-Timing API to better match the Server-Timing header spec by allowing metrics to include an optional description (desc) and by making duration (dur) optional when formatting header segments.

Changes:

  • Add description support to Perflab_Server_Timing_Metric via new setter/getter.
  • Update Server-Timing header formatting to conditionally include dur= and/or desc= and sanitize desc for safe header output.
  • Extend unit tests to cover duration+description combinations and escaping/CRLF stripping edge cases.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php Formats metric segments with optional dur and desc, including description sanitization.
plugins/performance-lab/includes/server-timing/class-perflab-server-timing-metric.php Adds a new $description field plus set_description() / get_description().
plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php Adds tests for header output with descriptions and special-character edge cases.
plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing-metric.php Adds unit tests for description getter/setter behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment thread plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php Outdated
Comment on lines 299 to 302
// If neither value nor description is set, skip this metric.
if ( null === $value && null === $description ) {
return null;
}

Copilot AI Feb 26, 2026

Copy link

Choose a reason for hiding this comment

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

The PR description mentions supporting a “name only” header format (e.g. wp-metric), but this implementation still skips metrics when both duration and description are null (returns null), so name-only metrics will never be emitted. If MDN compliance is the goal, consider returning the metric name when both are null and adding a corresponding test case.
Copilot uses AI. Check for mistakes.

@the-hercules the-hercules Feb 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll update it to remove the "name only" format since that was inaccurate.Metric with neither duration nor description provides no useful information in the header. This PR only adds description as a new optional field alongside the existing optional duration.

@westonruter westonruter Feb 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Metric with neither duration nor description provides no useful information in the header.

Is this so? In the MDN docs, this example is provided:

// A metric with a name only
Server-Timing: missedCache

@the-hercules the-hercules Feb 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@westonruter Added support for name only.
Comment thread plugins/performance-lab/includes/server-timing/class-perflab-server-timing-metric.php Outdated
Comment thread plugins/performance-lab/includes/server-timing/class-perflab-server-timing-metric.php
@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only labels Feb 26, 2026
@the-hercules

the-hercules commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

@westonruter I’ve added support for name-only metrics as suggested. When you get a chance, could you please take a look?
westonruter and others added 8 commits June 17, 2026 19:20
@westonruter
Merge branch 'trunk' into fix/allow-desc-dur-optional-879
1a557f1
@westonruter
Use native type hint for property
f659b47
@westonruter @claude
Use specific array shapes for Server-Timing description data providers
4627d0c 
Replace the vague `@return array<string, mixed>` on the new
`data_get_header_with_description()` and
`data_get_header_with_description_edge_cases()` data providers with the
precise `[expected, metrics]` tuple shape, reusing the imported
`MetricArguments` type.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@westonruter @claude
Add test for set_description late-setting guard
48c1eac 
Cover the `_doing_it_wrong` guard in `Perflab_Server_Timing_Metric::set_description()`, mirroring the existing `set_value` guard test. Default-metric registration is detached before firing `perflab_server_timing_send_header` so the test does not mutate the shared singleton.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@westonruter @claude
Treat empty Server-Timing metric description as unset
60e4888 
`set_description()` now normalizes an empty string to `null` so the metric is emitted without a `desc` parameter rather than an empty `desc=""`. The `@param` docblock is relaxed from `non-empty-string` to `string` to match the actual contract. Adds unit and `get_header()` coverage for the empty-description case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@westonruter
Tweak test_set_description_with_empty_string_is_treated_as_unset to e…
b33c699 
…nsure unset
@westonruter
Use non-empty-string for stored value and return value
19bc9a3
@westonruter @claude
Harden Server-Timing description sanitization
abe7684 
Strip the full C0 control range plus DEL (not just CR/LF) before escaping, since those characters are not representable in an RFC 7230 quoted-string. Also skip the `desc` parameter entirely when the description reduces to an empty string after sanitization, preserving the no-empty-desc invariant for control-character-only input. Adds coverage for both cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@westonruter westonruter merged commit 04960a9 into WordPress:trunk Jun 18, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature

3 participants

@the-hercules @westonruter