Repository navigation
Annotate the content membership experiment SQL with the repository (P… - #1651
Conversation
…ULP-1996) Add a second comment, `/* repo=<repository pk> */`, after each variant's `pexp=... v=A|B` marker in patch 0081, so RDS Performance Insights shows both the variant and the target repository. The existing marker is unchanged, so the experiment collector's pattern and the existing test assertions still match. Use self.repository_id, which is already on the RepositoryVersion, to avoid a database fetch of the repository on every call. Add a test assertion for the repo comment. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds repository identity to each content membership experiment SQL annotation using the existing repository ID, without changing the collector-facing variant marker, and verifies the new comment in the query test. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/tests/unit/test_interval_content_experiment.py" line_range="142" />
<code_context>
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} */"
+ assert any(repo_marker in query["sql"] for query in ctx.captured_queries), repo_marker
</code_context>
<issue_to_address>
**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.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: pulp_service/pulp_service/tests/unit/test_interval_content_experiment.py:142
| 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} */" |
There was a problem hiding this comment.
🟡 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`.Log the target repository on each ab_experiment_dispatch line, so CloudWatch counts can be split by repository, alongside the SQL annotation for RDS Performance Insights. run_lazy_experiment() takes an optional repository_id (null when not passed), and the get_content() call in patch 0081 passes self.repository_id. Add a test assertion for the field. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
/ok-to-test |
|
I tested the exact PR head in a built Validation:
... /* pexp=PULP-1996-CONTENT-MEMBERSHIP v=A */ /* repo=<repository UUID> */ ...
... /* pexp=PULP-1996-CONTENT-MEMBERSHIP v=B */ /* repo=<repository UUID> */ ...The format string therefore works at runtime and the repository comment survives into the executed SQL. A production/PI check is still needed after rollout to verify that Performance Insights retains and displays the new comment. One follow-up: the current experiment-results collector groups PI data by experiment, variant, and SQL ID, but does not yet parse the new |
kdelee
left a comment
There was a problem hiding this comment.
LGTM, not sure if we need that changelog fragment to be able to merge
Summary
Annotate the PULP-1996 experiment's SQL with the target repository, so RDS Performance Insights shows the variant and the repo. Follow-up from the #1643 review.
Each
pexp=... v=A|Bmarker in patch 0081 is now followed by/* repo=<repository pk> */. The existing marker is unchanged, so the collector pattern (#1650) and the existing tests still match. It usesself.repository_id, so there is no extra database fetch.Optional: repository on the dispatch log line
The
ab_experiment_dispatchlog line also gets arepository_idfield (second commit), so CloudWatch can count dispatches per repository and variant. This is separate from the SQL annotation and can be dropped if not wanted.run_lazy_experiment()takes an optionalrepository_id(null when not passed), and the call in patch 0081 passesself.repository_id.Testing
pexprepo=<id>Contentfiltered by A / Btrack_activity_query_size).ruff checkandruff format --checkpass.run_lazy_experiment()with stand-ins for the logger and Django: the dispatch line hasrepository_idwhen passed andnullotherwise, and the other fields are unchanged.Summary by Sourcery
Add repository context to content experiment SQL and dispatch telemetry while preserving existing experiment markers and behavior.
New Features:
Tests: