Skip to content

Annotate the content membership experiment SQL with the repository (P… #1651

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.

Sign up for GitHub

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

Merged
dkliban merged 2 commits into from
Oct 28, 2026
Merged

Annotate the content membership experiment SQL with the repository (P… #1651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81317e0
Annotate the content membership experiment SQL with the repository (P…
larrymou9 Oct 28, 2026
f558b2d
Add repository_id to the experiment dispatch log line (PULP-1996)
larrymou9 Oct 28, 2026
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
21 changes: 14 additions & 7 deletions images/assets/patches/0081-Experiment-interval-content-query.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
--- a/pulpcore/app/models/repository.py
+++ b/pulpcore/app/models/repository.py
@@ -31,6 +31,7 @@ from pulpcore.cache import Cache
@@ -31,6 +31,7 @@
from pulpcore.constants import ALL_KNOWN_CONTENT_CHECKSUMS, PROTECTED_REPO_VERSION_MESSAGE
from pulpcore.download.factory import DownloaderFactory
from pulpcore.exceptions import ContentOverwriteError, ResourceImmutableError
+from pulp_service.app.experiments import run_lazy_experiment

from .base import BaseModel, MasterModel
from .content import Artifact, Content, ContentArtifact, RemoteArtifact
@@ -1029,7 +1030,16 @@ class RepositoryVersion(BaseModel):
@@ -1064,7 +1065,17 @@
if content_qs is None:
content_qs = Content.objects

Expand All @@ -22,11 +22,12 @@
+ "PULP-1996-CONTENT-MEMBERSHIP",
+ control=lambda: content_qs.filter(pk__in=self.content_ids_subquery()),
+ candidate=lambda: content_qs.filter(pk__in=self.content_pks_subquery()),
+ repository_id=self.repository_id,
+ )

def content_ids_subquery(self):
"""
@@ -1044,10 +1054,43 @@ class RepositoryVersion(BaseModel):
@@ -1079,10 +1090,49 @@
"""
return (
RepositoryVersion.objects.filter(pk=self.pk)
Expand All @@ -35,8 +36,11 @@
+ cids=Func(
+ F("content_ids"),
+ function="unnest",
+ # Marker so variant A is identifiable in RDS Performance Insights Top SQL
+ template="unnest(%(expressions)s) /* pexp=PULP-1996-CONTENT-MEMBERSHIP v=A */",
+ # Variant A and target repo markers, read from RDS Performance Insights
+ template=(
+ "unnest(%(expressions)s) /* pexp=PULP-1996-CONTENT-MEMBERSHIP v=A */"
+ f" /* repo={self.repository_id} */"
+ ),
+ )
+ )
.values_list("cids", flat=True)
Expand All @@ -61,8 +65,11 @@
+ .annotate(
+ cid=Func(
+ F("content_id"),
+ # Marker so variant B is identifiable in RDS Performance Insights Top SQL
+ template="%(expressions)s /* pexp=PULP-1996-CONTENT-MEMBERSHIP v=B */",
+ # Variant B and target repo markers, read from RDS Performance Insights
+ template=(
+ "%(expressions)s /* pexp=PULP-1996-CONTENT-MEMBERSHIP v=B */"
+ f" /* repo={self.repository_id} */"
+ ),
+ )
+ )
+ .values_list("cid", flat=True)
Expand Down
6 changes: 4 additions & 2 deletions pulp_service/pulp_service/app/experiments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,16 @@ def run_experiment(exp_id, control, candidate, *, p_candidate, context):
logger.info(json.dumps(record))


def run_lazy_experiment(exp_id, control, candidate, *, p_candidate=0.5):
def run_lazy_experiment(exp_id, control, candidate, *, p_candidate=0.5, repository_id=None):
"""Dispatch one variant of an A/B experiment over lazily evaluated querysets.

control and candidate are zero-argument callables returning equivalent lazy
querysets. Unlike run_experiment(), no duration or result count is recorded:
building the queryset does not execute it, and callers may chain further
filters before evaluation. Latency is attributed per variant through the SQL
marker comments in RDS Performance Insights; this log line records only the
dispatch decision for volume and correlation analysis.
dispatch decision for volume and correlation analysis, plus the target
repository when the caller passes repository_id.
"""
variant = "B" if random.random() < p_candidate else "A" # noqa: S311
logger.info(
Expand All @@ -57,6 +58,7 @@ def run_lazy_experiment(exp_id, control, candidate, *, p_candidate=0.5):
"exp_id": exp_id,
"variant": variant,
"p_candidate": p_candidate,
"repository_id": str(repository_id) if repository_id else None,
"correlation_id": get_guid(),
}
)
Expand Down
5 changes: 4 additions & 1 deletion pulp_service/pulp_service/tests/unit/test_interval_content_experiment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,11 @@ def test_get_content_experiment_dispatch(repository, content_pks, add_content, m
dispatch["event"] == "ab_experiment_dispatch" and dispatch["exp_id"] == EXPERIMENT_ID for dispatch in dispatches
)
assert all(dispatch.get("correlation_id") for dispatch in dispatches)
assert all(dispatch.get("repository_id") == str(repository.pk) for dispatch in dispatches)


def test_get_content_marks_sql_per_variant(repository, content_pks, add_content, monkeypatch): # noqa: ARG001
"""Both variants must reach the database with their RDS Performance Insights marker."""
"""Both variants reach the database with their variant and repository markers."""
with repository.new_version() as version:
add_content(version, (1, 1, 1, 1, 1))

Expand All @@ -139,3 +140,5 @@ def test_get_content_marks_sql_per_variant(repository, content_pks, add_content,
with CaptureQueriesContext(connection) as ctx:
assert list(version.get_content())
assert any(marker in query["sql"] for query in ctx.captured_queries), marker
repo_marker = f"/* repo={repository.pk} */"

@sourcery-ai sourcery-ai Bot Oct 28, 2026

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.

🟡 Medium · Release note fragment missing

When this non-trivial SQL annotation change is merged, the SQL annotation change and its test assertion are included without a Towncrier fragment, so the required release note is absent.

Add a CHANGES/{issue_number}.{category} fragment for the change.

Prompt for AI agents 
In `pulp_service/pulp_service/tests/unit/test_interval_content_experiment.py` at line 142:

**Release note fragment missing**

When this non-trivial SQL annotation change is merged, the SQL annotation change and its test assertion are included without a Towncrier fragment, so the required release note is absent.

Add a `CHANGES/{issue_number}.{category}` fragment for the change.

From CLAUDE.md L76:

Uses **towncrier**. For any non-trivial change, create a file in `CHANGES/` named `{issue_number}.{category}` where category is one of: `feature`, `bugfix`, `doc`, `removal`, `deprecation`, `misc`.

assert any(repo_marker in query["sql"] for query in ctx.captured_queries), repo_marker
Loading