Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: do not output timeseries whose resource_type does not match an allowlist #78

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nadiamoe
Copy link
Member

@nadiamoe nadiamoe commented Mar 5, 2025

First take at browser metric filtering. This PR adds an allowlist of values for the resource_type tag, which k6 browser uses to indicate what prompted a request.

The code roughly works as follows: After removing timeseries by metric name as it was previously done, the list of timeseries is walked again. If a timeseries contains the resource_type label, and the value for that label is not present in the allowlist, the timeseries is yeeted. The allowlist is case-insensitive.

The allowlist itself defaults to just document, which means HTTP requests initiated by loading a webpage. This allowlist can be configured using the SM_K6_BROWSER_RESOURCE_TYPES environment variable, as a comma-separated list of valid resource_types. The special value *, if present, allows all resource_types and thus skips this second walk entirely.

A couple of integration tests, with different levels of granularity are added, both to cover the removal functionality and the handling of the SM_K6_BROWSER_RESOURCE_TYPES variable.

The mechanism by which a user can specify this allowlist in the SM UI/API and then have that value traverse its way here is out of the scope of this PR. Whether we want to merge this (breaking) change before or after merging this is also out of scope: If we chose to merge this first, then we can make the change non-breaking to users by setting SM_K6_BROWSER_RESOURCE_TYPES=* in the runner.

@nadiamoe nadiamoe force-pushed the browser-metrics-restrict branch from a406a4a to 956cf35 Compare March 5, 2025 15:51
Base automatically changed from k6-0-57-0 to main March 5, 2025 15:52
Paving the ground for testing more than one script
@nadiamoe nadiamoe force-pushed the browser-metrics-restrict branch 4 times, most recently from 9487dd4 to d7f78d8 Compare March 5, 2025 17:10
@nadiamoe nadiamoe changed the base branch from main to integration-browser-2 March 5, 2025 17:12
@nadiamoe nadiamoe force-pushed the browser-metrics-restrict branch from d7f78d8 to 172d78a Compare March 5, 2025 17:12
@nadiamoe nadiamoe force-pushed the browser-metrics-restrict branch from 172d78a to 4965c0b Compare March 5, 2025 17:26
@nadiamoe nadiamoe requested a review from d0ugal March 5, 2025 17:30
@nadiamoe nadiamoe marked this pull request as ready for review March 5, 2025 17:37
@nadiamoe nadiamoe requested a review from a team as a code owner March 5, 2025 17:37
@nadiamoe nadiamoe requested review from ka3de and removed request for a team March 5, 2025 17:37
Copy link
Contributor

@d0ugal d0ugal left a comment

Choose a reason for hiding this comment

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

This seems reasonable but its not clear to me how/where this will be used?

@nadiamoe
Copy link
Member Author

nadiamoe commented Mar 6, 2025

@d0ugal I don't think that's decided yet. My personal preference is to enforce this default everywhere, meaning this change will propagate to every browser check in there automatically. As we're in private preview, we can get away with this, and furthermore this will fix many browser scripts that are seeing their metrics truncated as they produce too many timeseries. In the future, if people request this, we can allow end users to tweak this allowlist by adding a field for this in the check settings.

If for some reason we don't want to do the breaking part, there's also a nonbreaking route:

  • Set SM_K6_BROWSER_RESOURCE_TYPES=* in the runner when we update xk6-sm to the version carrying this change (effectively negating the change)
  • Develop the UI, API, Agent, and runner plumbing to allow users to specify this in the script settings, defaulting to "only document" for new checks.
  • Use that property of the script to set SM_K6_BROWSER_RESOURCE_TYPES in the runner

@nadiamoe nadiamoe force-pushed the integration-browser-2 branch from 1b1763c to 3830589 Compare March 6, 2025 14:22
Base automatically changed from integration-browser-2 to main March 11, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants