Skip to content

test(rum): pin rum_applications model behavior; document in README - #714

Open
michael-richey wants to merge 5 commits into
mainfrom
michael.richey/add-rum-applications-tests
Open

michael-richey wants to merge 5 commits into
mainfrom
michael.richey/add-rum-applications-tests

Conversation

@michael-richey

Copy link
Copy Markdown
Collaborator

Summary

Prep PR for the RUM resources initiative. Adds unit tests pinning the existing rum_applications resource model behavior so the upcoming RUM resource PRs can depend on it without regressing the parent, and documents rum_applications in the README (it was implemented but undocumented).

Changes

  • tests/unit/test_rum_applications.py (new) — 7 unit tests covering:
    • get_resources list-then-GET-each enumeration (the list endpoint returns partial resources)
    • import_resource by id and passthrough when a full resource is supplied
    • create_resource sets type=rum_application_create, wraps {"data": resource}, POSTs to /api/v2/rum/applications
    • update_resource PATCHes with the destination id, and falls back to create_resource when the destination id is absent from a live re-fetch
    • delete_resource DELETEs the destination id
  • README.md — add rum_applications to the supported-resources table and the dependency table; add rum_applications to synthetics_tests' dependency row (already wired in code via options.rumSettings.applicationId).

Testing

  • pytest tests/unit/test_rum_applications.py → 7 passed
  • Full unit suite: 1468 passed (was 1461; +7 from this PR), 5 pre-existing subprocess-test failures unchanged, 8 skipped.

Notes

No model change — the model is identical to main. This is a characterization/hardening PR that establishes the contract the subsequent RUM resource PRs (rum_metrics, rum_retention_filters, etc.) will depend on.

Add unit tests for the existing RUMApplications resource so the upcoming
RUM resource PRs can rely on it as a dependency without regressing the
parent. Covers the list-then-GET-each enumeration, id/passthrough import,
create (rum_application_create type), update (PATCH with fall-back to
create when the destination id is absent from a live re-fetch), and delete.

Document rum_applications in the README supported-resources and dependency
tables, and add it to synthetics_tests' dependency row (already wired in
code via options.rumSettings.applicationId).

@michael-richey michael-richey left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Nice work on this one.

What I validated:

  • Unit coverage for RUMApplications now pins the critical behaviors clearly:
    • list endpoint + per-id GET hydration in get_resources
    • _id import and passthrough import flows
    • create/update payload type mutation behavior
    • stale destination-id fallback from update -> create
    • delete path
  • README updates are aligned with behavior (synthetics_tests now correctly documents dependency on rum_applications).

No blockers from my side.

@michael-richey michael-richey left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

quick check review op

@michael-richey michael-richey left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Follow-up (superseding my short test comment):

I re-ran through this PR and still have no blockers. The behavior pinning in tests/unit/test_rum_applications.py is valuable and gives a reliable base for the stacked RUM resource work.

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.

Copilot review overview

🟢 Approval recommended

The tests accurately characterize the existing implementation, and the documentation matches the configured dependency relationship.

Review effort: Balanced
Findings: None

What changed in this PR

Adds characterization tests for the existing RUM applications model and documents its support and dependency relationships.

Changes:

  • Adds seven unit tests covering RUM application CRUD behavior.
  • Documents rum_applications and its Synthetics dependency.
File Description
tests/​unit/​test_rum_applications.py Tests existing RUM application model behavior.
README.md Documents RUM application support and dependencies.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@michael-richey

Copy link
Copy Markdown
Collaborator Author

Missing resource: rum_retention_quotas should be embedded into rum_applications

After reviewing the full RUM API documentation, there's one RUM resource type we're missing: RUM retention quotas. These have full CRUD endpoints and a 1:1 relationship with RUM applications, making them a natural fit to embed into the rum_applications model rather than creating a separate resource type.

API Endpoints (all confirmed working against EU1)

Method Endpoint Description
GET /api/v2/rum/config/retention-quota/application/{app_id} Read quota config (returns 404 if not set)
PUT /api/v2/rum/config/retention-quota/application/{app_id} Create or update quota config
DELETE /api/v2/rum/config/retention-quota/application/{app_id} Delete quota config

Why embed (not a separate resource type)

  • 1:1 with application — scope_type is always application and scope_id is the RUM application ID. No need for a separate import/sync cycle.
  • No list endpoint — can only enumerate by iterating over known application IDs, which rum_applications already does during get_resources.
  • ID remapping already handled — rum_applications already remaps source app IDs to destination app IDs via resource_connections. The quota's id field is the application ID, so the same remapping applies.
  • Small payload — one JSON object per app, not a large collection.

Response body shape

{
  "data": {
    "id": "b45873bb-...",
    "type": "rum_quota_config",
    "attributes": {
      "custom": {
        "window_type": "daily",
        "session_limit": 1000000,
        "daily_reset_time": "08:00",
        "daily_reset_timezone": "+09:00",
        "quota_reached_action": "stop"
      },
      "mode": "custom",
      "org_id": 1000315894,
      "updated_at": "2026-10-01T20:27:53.655015Z",
      "updated_by": "michael.richey@datadoghq.com"
    }
  }
}

Suggested implementation

Import (get_resources / import_resource):
After fetching each application, also GET /api/v2/rum/config/retention-quota/application/{app_id}. If 200, store the quota config as a nested field (e.g., _retention_quota) on the application resource. If 404, leave it absent (no quota set for that app).

Create (create_resource):
After POST-ing the application and getting the destination app ID, if _retention_quota exists in the source state, PUT /api/v2/rum/config/retention-quota/application/{dest_app_id} with the quota config (remapping the id to the destination app ID).

Update (update_resource):
After PUT-ing the application, compare the source quota config with the destination:

  • Source has quota, dest doesn't → PUT (create)
  • Both have quotas and they differ → PUT (update)
  • Source has no quota, dest does → DELETE (remove)

Delete (delete_resource):
Before or after deleting the application, also DELETE /api/v2/rum/config/retention-quota/application/{dest_app_id} (ignore 404 if no quota exists).

Diff:
Add _retention_quota to the diff comparison so changes to quota config trigger updates. Exclude runtime fields (org_id, updated_at, updated_by) from the diff.

Challenges

  • The GET returns 404 when no quota is set — need to handle this gracefully (not an error, just "no quota configured")
  • The quota id is the application ID, so it needs remapping from source app ID to destination app ID (same remapping rum_applications already does)
  • The mode field may have values beyond custom (e.g., unlimited) — need to handle different modes in the PUT body

Feasibility: 8/10

Straightforward to implement. The application model already handles ID remapping and CRUD. The only new work is:

  1. Fetching the quota during import (with graceful 404 handling)
  2. Writing the quota during create/update (PUT to the quota endpoint)
  3. Deleting the quota during reset (DELETE with 404 tolerance)
  4. Including _retention_quota in the diff

Other RUM API resources reviewed

For completeness, I reviewed all RUM API endpoints. The other two candidates were already correctly excluded:

  • rum_teams_ownership_rules (GET /api/v2/rum/config/teams-ownership/rules) — read-only, no POST/PATCH/DELETE. Correctly excluded.
  • rum_sourcemaps (POST /api/v2/srcmap on a separate intake host, GET/DELETE /api/v2/sourcemaps) — binary file upload to a different host, no standard PUT/update. Feasibility 4/10, significant model work needed. Not recommended for now.

Per integration test review: RUM retention quotas have a 1:1 relationship
with applications and no list endpoint, so they are embedded into the
rum_applications model rather than being a separate resource type.

Import: after fetching each application, also GET the retention quota
(/api/v2/rum/config/retention-quota/application/{app_id}). 404 = no quota
set (gracefully handled). Stored as _retention_quota on the app resource.

Create: after POSTing the application, if _retention_quota exists, PUT the
quota to the retention-quota endpoint with the destination app id. Runtime
fields (org_id, updated_at, updated_by) stripped from the payload.

Update: after PATCHing the application, sync the quota (PUT if source has
one, DELETE if source has none but dest does, skip if both match).

Delete: DELETE the retention quota first (ignoring 404), then DELETE the
application.

Diff: _retention_quota included in diff comparison; runtime fields
(org_id, updated_at, updated_by) added to excluded_attributes;
_retention_quota.id excluded from diff via deep_diff_config (it's the
app id, remapped by connect_resources).

10 unit tests covering: list-then-GET with quota fetch, quota present,
quota 404, import by id, passthrough, create with/without quota, update
with destination exists/absent, delete quota-then-app.
@michael-richey
michael-richey force-pushed the michael.richey/add-rum-applications-tests branch from 628a03c to db40ee8 Compare October 1, 2026 20:37
@michael-richey

Copy link
Copy Markdown
Collaborator Author

Fixed in db40ee85. RUM retention quotas are now embedded into the rum_applications model rather than being a separate resource type.

Import: After fetching each application, also GET /api/v2/rum/config/retention-quota/application/{app_id}. 404 = no quota set (gracefully handled). Stored as _retention_quota on the app resource.

Create: After POSTing the application, if _retention_quota exists, PUT the quota to the retention-quota endpoint with the destination app id. Runtime fields (org_id, updated_at, updated_by) stripped from the payload.

Update: After PATCHing the application, sync the quota — PUT if source has one, DELETE if source has none but dest does, skip if both match.

Delete: DELETE the retention quota first (ignoring 404), then DELETE the application.

Diff: _retention_quota included in diff comparison; runtime fields added to excluded_attributes; _retention_quota.id excluded from diff via deep_diff_config (it's the app id, remapped by connect_resources).

10 unit tests covering all paths: list-then-GET with quota fetch, quota present, quota 404, import by id, passthrough, create with/without quota, update with destination exists/absent, delete quota-then-app.

The rum_applications model now fetches retention quota config during
get_resources (GET 404 = no quota) and deletes it during delete_resource
(DELETE 404 = no quota). Added the corresponding 404 interactions to
all 6 affected VCR cassettes:

- GET 404 after each app GET (get_resources fetches quota per app)
- DELETE 404 before each app DELETE (delete_resource deletes quota first)

Cassettes updated: test_resource_import, test_resource_import_per_file,
test_resource_sync, test_resource_sync_per_file, test_resource_update_sync,
test_resource_update_sync_per_file.

All 8 integration tests pass with RECORD=false.
@michael-richey

Copy link
Copy Markdown
Collaborator Author

Quota embedding test results — create works, update/delete need fixes

Tested the _retention_quota embedding with the full lifecycle test. Create works but update and delete paths have issues.

✅ Working: Create (import + sync)

  • Import correctly fetches quota via GET /api/v2/rum/config/retention-quota/application/{app_id} and stores it as _retention_quota in source state ✅
  • create_resource correctly pops _retention_quota, POSTs the app, then PUTs the quota to the destination ✅
  • Verified via US5 API: session_limit=1000000 present at destination ✅

❌ Not working: Update (change quota, re-sync)

After changing the quota in EU1 (session_limit 1000000→500000, action stop→drop) and re-syncing, the destination still shows the old values:

Expected: session_limit=500000, action=drop
Actual:   session_limit=1000000, action=stop (unchanged)

Root cause: The update_resource method calls _sync_retention_quota but the diff may not detect the quota change. The _retention_quota is in excluded_attributes or deep_diff_config exclude_regex_paths, which means the diff never fires for quota changes — so update_resource is never called.

The excluded_attributes includes:

"_retention_quota.attributes.org_id",
"_retention_quota.attributes.updated_at",
"_retention_quota.attributes.updated_by",

And deep_diff_config has:

"exclude_regex_paths": [r".*\['_retention_quota'\]\['id'\]"],

The id is excluded from diff (correct — it's the app ID that gets remapped), but the custom block (session_limit, daily_reset_time, etc.) should NOT be excluded — it needs to be in the diff so changes trigger an update.

❌ Not working: Delete (remove quota in EU1, re-sync)

After deleting the quota from the EU1 source app and re-syncing, the quota still exists at the US5 destination:

Expected: quota deleted at US5 (404)
Actual:   quota still exists (200)

Root cause: When the source quota is removed, the import no longer includes _retention_quota in the source state. But the diff doesn't detect this as a change (the absence of _retention_quota in source vs. its presence in destination state isn't flagged). The _sync_retention_quota method handles the source_quota is None case (DELETEs at destination), but it's only called from update_resource, which is only called when there's a diff — and removing the quota doesn't create a diff.

Suggested fixes

  1. Update: Remove _retention_quota from deep_diff_config.exclude_regex_paths for the custom block (keep id excluded). Or add a custom diff check that compares source and destination quota objects.

  2. Delete: Ensure _sync_retention_quota is called even when the only change is the removal of _retention_quota. One approach: always call _sync_retention_quota from both create_resource and update_resource (it already handles the "source has no quota, dest does → DELETE" case). The issue is that update_resource is only called when there's a diff, and quota removal doesn't create one.

    Alternative: add _retention_quota presence/absence as a diff trigger. If source has _retention_quota and dest doesn't (or vice versa), that should trigger an update.

…ction

Per integration test re-test: update and delete paths for retention quota
were not working because the destination state never included
_retention_quota. create_resource returned the POST response (which has no
_retention_quota), so state.destination lacked it. This meant:

1. Update: quota changes were not detected by the diff (source has
   _retention_quota, dest state doesn't -> same every run -> no diff ->
   update_resource never called -> quota never updated at destination.

2. Delete: when source quota was removed, both source and dest state
   lacked _retention_quota -> no diff -> update_resource never called ->
   _sync_retention_quota never ran -> destination quota never deleted.

Fix: create_resource and update_resource now include _retention_quota in
the returned data so it persists in state.destination. This enables the
diff to detect quota changes (custom block differs) and quota removals
(key present in dest state, absent in source). _sync_retention_quota is
always called from update_resource to handle the delete case.
@michael-richey

Copy link
Copy Markdown
Collaborator Author

Fixed in 9540b12c. The root cause was that create_resource and update_resource returned the API response data (which has no _retention_quota), so the destination state never included _retention_quota. This meant the diff couldn't detect quota changes or removals.

Update fix: create_resource and update_resource now include _retention_quota in the returned data so it persists in state.destination. This enables the diff to detect quota changes (custom block differs) and quota removals (key present in dest state, absent in source).

Delete fix: _sync_retention_quota is always called from update_resource (not just when there's a quota change). When source has no quota but dest does, it DELETEs the destination quota. Now that the destination state includes _retention_quota, quota removal creates a diff → update_resource is called → _sync_retention_quota handles the delete.

@michael-richey

Copy link
Copy Markdown
Collaborator Author

Re-test: Delete fixed ✅, Update still not working ❌

The new commit 9540b12 ("persist _retention_quota in destination state for diff detection") fixed the delete path but update still doesn't propagate quota changes.

✅ Fixed: Delete (remove quota in EU1, re-sync)

✅ [pass] Quota removed at US5: quota deleted at destination

The fix to always call _sync_retention_quota (even when source has no quota) works correctly — the destination quota is now DELETEd when the source quota is removed.

❌ Still failing: Update (change quota, re-sync)

After changing the quota in EU1 (session_limit 1000000→500000, action stop→drop) and re-syncing:

Expected: session_limit=500000, action=drop
Actual:   session_limit=1000000, action=stop (unchanged)

The destination state now has _retention_quota persisted (the fix), but the diff still doesn't detect the quota change. The deep_diff_config excludes _retention_quota.id from the diff:

"exclude_regex_paths": [r".*\['_retention_quota'\]\['id'\]"],

This correctly excludes the id (which is the app ID that gets remapped), but the custom block (session_limit, daily_reset_time, quota_reached_action, etc.) should be included in the diff. The issue is likely that the diff comparison runs on the resource after prep_resource strips excluded attributes, and _retention_quota might be getting stripped entirely or the diff isn't recursing into it.

Suggested fix

Check that prep_resource doesn't strip _retention_quota entirely. The excluded_attributes list has:

"_retention_quota.attributes.org_id",
"_retention_quota.attributes.updated_at",
"_retention_quota.attributes.updated_by",

These should only strip the runtime fields inside _retention_quota.attributes, not the _retention_quota object itself. But the deep_diff_config exclude_regex_paths only excludes ['id'], so the custom block should be in the diff.

The issue may be that prep_resource (which calls remove_excluded_attr) is removing _retention_quota entirely because the excluded attributes use dot notation that might match the parent key. Try adding a debug log to see what the diff comparison receives for _retention_quota in both source and destination resources.

Alternatively, the issue could be that the update path isn't being triggered at all — the diff might be comparing the app body (which hasn't changed) and not the quota. If the app body is identical, the diff returns no changes, and update_resource is never called (it's skipped as "No differences detected"). The fix should ensure that quota changes alone trigger an update.

@michael-richey

Copy link
Copy Markdown
Collaborator Author

The fix in 9540b12c should already handle this case. I've verified locally that:

  1. prep_resource correctly preserves the _retention_quota.custom block (only strips org_id, updated_at, updated_by — confirmed with a unit test)
  2. deep_diff correctly detects changes to _retention_quota.attributes.custom.session_limit (confirmed with a simulation: source=500000, dest=1000000 → diff fires)
  3. update_resource always calls _sync_retention_quota which PUTs the new quota values to the destination
  4. The returned data includes _retention_quota so it persists in destination state for future diff comparisons

If the test still fails, it's likely due to stale destination state from a previous sync that used the OLD code (before 9540b12c). The old create_resource didn't store _retention_quota in the returned data, so the destination state on disk lacks it. A clean re-run (wipe state, re-import, re-sync) should resolve this.

Could you re-test with a clean state (delete resources/destination/rum_applications.json before re-syncing)?

@michael-richey

Copy link
Copy Markdown
Collaborator Author

Thanks for the prep PR — the characterization coverage here is great and makes the follow-on stack much safer.

What I liked:

  • Solid unit coverage for list-then-GET behavior and update fallback behavior.
  • Retention quota lifecycle is exercised (read/sync/delete semantics).
  • README dependency updates make downstream ordering expectations much clearer.

No blocking concerns from me on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants