feat(data-warehouse): implement the aws compute optimizer import source - #110795
Conversation
|
😎 Merged successfully - details. |
Gilbert09
added
the
stamphog
Talyn App
|
Risk: No findings The only change since the last review moves the frozen test fixture dates in Sentinel reviewed |
|
Hey @Gilbert09! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
🤖 CI report
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/acculynx/source.py:18 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/source.py:22 |
14 | 137 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/aws_compute_optimizer.py:57 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_glue_data_catalog/aws_glue_data_catalog.py:70 |
11 | 84 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/aws_compute_optimizer.py:109 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_glue_data_catalog/aws_glue_data_catalog.py:113 |
11 | 70 |
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
⚠️ Backend coverage — 98.0% of changed backend lines covered — 7 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ████████████████████ 98.0% (353 / 360)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/source.py |
88.5% | 78, 83, 91 |
products/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/aws_compute_optimizer.py |
97.2% | 60–61, 82, 131 |
🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 54609947516370 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 53.4% |
1,445 / 2,707 |
batch_exports |
████████████████░░░░ 81.3% |
21,583 / 26,561 |
cdp |
██████████████████░░ 88.3% |
4,559 / 5,164 |
mcp_analytics |
██████████████████░░ 89.2% |
5,038 / 5,651 |
product_tours |
██████████████████░░ 89.3% |
1,340 / 1,500 |
dashboards |
██████████████████░░ 89.6% |
6,924 / 7,727 |
notebooks |
██████████████████░░ 90.2% |
15,514 / 17,207 |
signals |
██████████████████░░ 90.4% |
60,188 / 66,556 |
cohorts |
██████████████████░░ 90.5% |
8,534 / 9,434 |
data_warehouse |
██████████████████░░ 90.6% |
14,375 / 15,860 |
streamlit_apps |
██████████████████░░ 90.8% |
2,684 / 2,956 |
managed_warehouse |
██████████████████░░ 91.0% |
10,252 / 11,263 |
data_modeling |
██████████████████░░ 91.2% |
10,562 / 11,584 |
tasks |
██████████████████░░ 91.3% |
79,748 / 87,306 |
exports |
██████████████████░░ 91.7% |
9,684 / 10,566 |
business_knowledge |
██████████████████░░ 92.0% |
8,472 / 9,208 |
engineering_analytics |
██████████████████░░ 92.2% |
11,497 / 12,475 |
today |
██████████████████░░ 92.3% |
999 / 1,082 |
early_access_features |
███████████████████░ 92.6% |
1,339 / 1,446 |
conversations |
███████████████████░ 92.6% |
29,225 / 31,559 |
stamphog |
███████████████████░ 92.8% |
8,109 / 8,742 |
canvas |
███████████████████░ 92.9% |
7,155 / 7,703 |
approvals |
███████████████████░ 93.0% |
3,974 / 4,271 |
mcp_registry |
███████████████████░ 93.1% |
1,670 / 1,794 |
notifications |
███████████████████░ 93.2% |
1,144 / 1,228 |
error_tracking |
███████████████████░ 93.3% |
16,389 / 17,573 |
surveys |
███████████████████░ 93.4% |
6,644 / 7,113 |
autoresearch |
███████████████████░ 93.4% |
8,837 / 9,457 |
slack_app |
███████████████████░ 93.7% |
14,611 / 15,600 |
context_layer |
███████████████████░ 93.8% |
3,415 / 3,639 |
web_analytics |
███████████████████░ 93.9% |
23,680 / 25,229 |
billing_alerts |
███████████████████░ 94.1% |
2,094 / 2,226 |
mcp_store |
███████████████████░ 94.3% |
8,959 / 9,501 |
alerts |
███████████████████░ 94.7% |
9,319 / 9,844 |
wizard |
███████████████████░ 94.7% |
6,150 / 6,496 |
workflows |
███████████████████░ 94.7% |
15,187 / 16,034 |
ai_observability |
███████████████████░ 94.7% |
26,034 / 27,482 |
reminders |
███████████████████░ 94.8% |
760 / 802 |
review_hog |
███████████████████░ 95.0% |
11,750 / 12,362 |
annotations |
███████████████████░ 95.1% |
817 / 859 |
endpoints |
███████████████████░ 95.1% |
9,234 / 9,706 |
customer_analytics |
███████████████████░ 95.2% |
26,116 / 27,428 |
marketing_analytics |
███████████████████░ 95.3% |
19,450 / 20,413 |
posthog_ai |
███████████████████░ 95.4% |
2,530 / 2,653 |
actions |
███████████████████░ 95.5% |
756 / 792 |
logs |
███████████████████░ 95.5% |
15,468 / 16,200 |
experiments |
███████████████████░ 95.5% |
33,000 / 34,548 |
data_catalog |
███████████████████░ 95.5% |
4,401 / 4,606 |
tracing |
███████████████████░ 95.6% |
3,536 / 3,699 |
replay_vision |
███████████████████░ 95.6% |
29,389 / 30,727 |
growth |
███████████████████░ 95.7% |
11,381 / 11,888 |
skills |
███████████████████░ 95.8% |
6,972 / 7,274 |
messaging |
███████████████████░ 95.9% |
3,834 / 3,999 |
product_analytics |
███████████████████░ 96.0% |
28,521 / 29,696 |
revenue_analytics |
███████████████████░ 96.4% |
1,889 / 1,959 |
user_interviews |
███████████████████░ 96.5% |
2,870 / 2,974 |
feature_flags |
███████████████████░ 96.6% |
27,119 / 28,060 |
access_control |
███████████████████░ 96.7% |
7,739 / 8,007 |
warehouse_sources |
███████████████████░ 97.3% |
473,734 / 486,821 |
data_quality |
████████████████████ 97.5% |
7,701 / 7,895 |
metrics |
████████████████████ 98.0% |
4,252 / 4,338 |
analytics_platform |
████████████████████ 98.3% |
2,784 / 2,833 |
pulse |
████████████████████ 98.5% |
2,046 / 2,078 |
live_debugger |
████████████████████ 99.2% |
626 / 631 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
🧰 Additional context used📚 Code guidelines (8)📝 WalkthroughWalkthroughAdds an AWS Compute Optimizer source for six recommendation datasets. The source sends SigV4-signed requests, normalizes responses, validates credentials and enrollment, and supports resumable pagination with saved state. It also defines endpoint schemas and configuration fields, and adds request, pagination, and validation tests. Ingestion warning tests now use July 2099 timestamps and corresponding time bounds. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to AWS Compute Optimizer connections in GovCloud will fail, and malformed service errors can cause unexpected import failures. Address these risks before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The integration uses constrained AWS endpoints, signed requests, credential-redaction inputs and existing ingestion lifecycle controls. No introduced security flaw was established. Remaining uncertainty concerns credential handling beyond the connector and recovery under concurrent attempts or downstream failures. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/aws_compute_optimizer.py-132-134 (1)
132-134: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the shape of the
errorselement.
result["errors"][0]assumes the element is a dict. If AWS returns a non-dict element,error.getraisesAttributeError. That error escapes theexcept (requests.RequestException, ValueError)handler invalidate_credentials, and the user gets no actionable message. Check the type and fall back toPartialResponseError.Proposed fix
- error = result["errors"][0] - raise AwsComputeOptimizerError(str(error.get("code") or "PartialResponseError")) + errors = result["errors"] + error = errors[0] if isinstance(errors, list) and errors else None + code = error.get("code") if isinstance(error, dict) else None + raise AwsComputeOptimizerError(str(code or "PartialResponseError"))Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 16337112-b5ec-4a1c-a9fe-4eb4d2f5604b
📒 Files selected for processing (7)
products/warehouse_sources/backend/temporal/data_imports/sources/SOURCES.mdproducts/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/aws_compute_optimizer.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/canonical_descriptions.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/settings.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/source.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/tests/test_aws_compute_optimizer.pyproducts/warehouse_sources/backend/temporal/data_imports/sources/generated_configs/awscomputeoptimizer.py
Limit details: You’ve used all 12 included reviews currently available.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
CI is fully green and there are no unresolved review threads or merge conflicts. The remaining merge gate is approval from Team Warehouse Sources; please review and approve when ready. 🦉 via talyn.dev |
Rebase the AWS Compute Optimizer source onto master and preserve the source catalog changes from both branches.
6fe56eb to
14b73a1
Compare
A new stamphog review started for this PR — the fresh verdict replaces this approval.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Re-run CI after the duplication lint job was cancelled without logs. Includes the conflict resolution that preserves both source catalog updates.
Retry CI after an unrelated ingestion warnings test failed with an inconsistent result count. The AWS Compute Optimizer source tree is unchanged.
|
CI retry note: the prior run failed only in 🦉 via talyn.dev |
|
/trunk merge |
Move the frozen ingestion-warning fixture dates far enough into the future that ClickHouse's wall-clock TTL cannot remove the wider-window row during the test run.
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
Additive new warehouse source by an owning-team author, with thorough tests, region input validation, no redirects, and secrets redacted in the tracked session. The unrelated ingestion-warnings test date shift is test-only and harmless.
- Author wrote 37% of the modified lines and has 104 merged PRs in these paths (familiarity MODERATE).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 522L, 5F substantive, 889L/8F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (889L, 8F, two-areas, feat) |
| stamphog 2.3.1 | .stamphog/policy.yml @ 1ffc43a · reviewed head 1ffc43a |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Use the FIPS endpoint for GovCloud regions. · aws_compute_optimizer.py:76-105
products/warehouse_sources/backend/temporal/data_imports/sources/aws_compute_optimizer/aws_compute_optimizer.py:76-105
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the FIPS endpoint for GovCloud regions.
For
us-gov-*, AWS exposes Compute Optimizer throughcompute-optimizer-fips.{region}.amazonaws.com. The client currently uses the non-FIPS hostname. Credential validation and recommendation syncs can fail for GovCloud users. Keep the SigV4 service and region unchanged.Suggested fix
- self.url = f"https://compute-optimizer.{self.region}.{suffix}/" + endpoint_prefix = "compute-optimizer-fips" if self.region.startswith("us-gov-") else "compute-optimizer" + self.url = f"https://{endpoint_prefix}.{self.region}.{suffix}/"Update the GovCloud test expectation to use the same FIPS hostname.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 264c39b3-4738-44fc-a5c2-a29aba8ad042
📒 Files selected for processing (1)
posthog/api/test/test_ingestion_warnings_v2.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 0 remain after this review.
|
Reviewed the latest automated concerns. GovCloud is already supported: 🦉 via talyn.dev |
|
/trunk merge |
1 similar comment
|
/trunk merge |
…v2 api test #110795, queued ahead of this PR, fixes the same TTL date bomb by moving the fixtures to 2099, and the earlier relative-date version here conflicted with it in the merge queue. Use the identical file contents so the two PRs merge cleanly in either order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 7cd74fea-b97a-44c0-a272-3a41767003e2
Deploy status
|
1 participant
Problem
Changes
ec2_instance_recommendationsauto_scaling_group_recommendationslambda_function_recommendationsecs_service_recommendationsebs_volume_recommendationsrecommendation_summariesaws_organizationssource.unreleasedSourceremoved.SOURCES.mdrow.How did you test this code?
Test rationale: the source is new, so no existing test covers its transport.
Release status
Automatic notifications
Docs update