feat(infrahubctl): add telemetry commands#937
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
eea7d24 to
95ab066
Compare
Deploying infrahub-sdk-python with
|
| Latest commit: |
83edc00
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5a765058.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://fac-ifc-2284-telemetry-ctl.infrahub-sdk-python.pages.dev |
95ab066 to
866cf4d
Compare
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #937 +/- ##
====================================================
- Coverage 82.63% 81.12% -1.51%
====================================================
Files 133 134 +1
Lines 13250 11329 -1921
Branches 2306 1699 -607
====================================================
- Hits 10949 9191 -1758
+ Misses 1652 1594 -58
+ Partials 649 544 -105
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes 🚀 New features to boost your workflow:
|
866cf4d to
2b9f378
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
infrahub_sdk/ctl/telemetry.py (3)
31-40: Consider extracting shared API call logic into a helper function.Both
list_snapshotsandexport_snapshotsshare nearly identical code for building parameters and fetching from the telemetry endpoint. A helper would reduce duplication and centralize error handling.Optional refactor
async def _fetch_telemetry_snapshots( client: InfrahubClient, start_date: str | None = None, end_date: str | None = None, limit: int = 50, ) -> dict: """Fetch telemetry snapshots from the API.""" params: dict[str, str | int] = {"limit": limit} if start_date: params["start_date"] = start_date if end_date: params["end_date"] = end_date url = f"{client.address}/api/telemetry/snapshots?{urlencode(params)}" response = await client._get(url=url, timeout=client.default_timeout) response.raise_for_status() return response.json()Also applies to: 78-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/ctl/telemetry.py` around lines 31 - 40, Extract the duplicated request-building and fetching logic from list_snapshots and export_snapshots into a new helper (suggest name _fetch_telemetry_snapshots) that accepts (client, start_date=None, end_date=None, limit=50), constructs the params dict, builds the URL, performs client._get with client.default_timeout, calls response.raise_for_status() and returns response.json(); then update list_snapshots and export_snapshots to call this helper and handle any result-specific processing so error handling and parameter construction are centralized.
78-78: Hardcoded limit may silently truncate large exports.The
limit: 10000is hardcoded, but if more snapshots exist, the export will be incomplete without warning. Consider:
- Adding a warning if
len(snapshots) == 10000(indicating possible truncation)- Extracting this as a named constant
- Comparing against
data.get('count')to detect truncationSuggested improvement
+EXPORT_MAX_LIMIT = 10000 + `@app.command`(name="export") `@catch_exception`(console=console) async def export_snapshots( output: str = typer.Option("telemetry-export.json", help="Output file path"), start_date: str | None = typer.Option(None, help="Start date filter (ISO 8601)"), end_date: str | None = typer.Option(None, help="End date filter (ISO 8601)"), _: str = CONFIG_PARAM, ) -> None: """Export telemetry snapshots to a JSON file.""" client = initialize_client() - params: dict[str, str | int] = {"limit": 10000} + params: dict[str, str | int] = {"limit": EXPORT_MAX_LIMIT} # ... rest of function ... snapshots = data.get("snapshots", []) + total_count = data.get("count", len(snapshots)) + if len(snapshots) < total_count: + console.print(f"[yellow]Warning: Exported {len(snapshots)} of {total_count} total snapshots (limit reached)[/yellow]")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/ctl/telemetry.py` at line 78, The hardcoded params: dict with "limit": 10000 can silently truncate exports; extract that magic number into a named constant (e.g., MAX_SNAPSHOT_LIMIT), use it when constructing params, and after fetching snapshots compare len(snapshots) to MAX_SNAPSHOT_LIMIT and also compare against data.get('count') when available; if len(snapshots) == MAX_SNAPSHOT_LIMIT or data.get('count', 0) > len(snapshots) emit a clear warning via the module logger so callers know the export may be incomplete (update the code paths around the params variable and the snapshot-fetching function in telemetry.py).
37-40: The telemetry commands useclient._get()to access raw HTTP endpoints (/api/telemetry/snapshots). While_get()is private, there is currently no public client method available for raw HTTP GET requests. The telemetry functionality requires direct HTTP access that isn't exposed through the GraphQL or node-based public API.Recommendation: If telemetry becomes a core SDK feature, consider either (a) adding public telemetry accessor methods to InfrahubClient or (b) providing a public HTTP utility method for endpoints that require raw HTTP access. For now, document this usage pattern to clarify why the private method is necessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/ctl/telemetry.py` around lines 37 - 40, Telemetry code is using the private client._get() to call raw HTTP endpoints (e.g., in infrahub_sdk/ctl/telemetry.py building url and calling client._get), which should be exposed or documented; add a public accessor on InfrahubClient (e.g., a method named http_get or raw_get that accepts url, timeout, params and forwards to the underlying HTTP client) and update telemetry code to call that public method (or alternatively add explicit public telemetry methods on InfrahubClient such as get_telemetry_snapshots to encapsulate the URL), and include a short docstring noting intended use for raw HTTP endpoint access so future callers avoid using _get directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@infrahub_sdk/ctl/telemetry.py`:
- Around line 31-40: Extract the duplicated request-building and fetching logic
from list_snapshots and export_snapshots into a new helper (suggest name
_fetch_telemetry_snapshots) that accepts (client, start_date=None,
end_date=None, limit=50), constructs the params dict, builds the URL, performs
client._get with client.default_timeout, calls response.raise_for_status() and
returns response.json(); then update list_snapshots and export_snapshots to call
this helper and handle any result-specific processing so error handling and
parameter construction are centralized.
- Line 78: The hardcoded params: dict with "limit": 10000 can silently truncate
exports; extract that magic number into a named constant (e.g.,
MAX_SNAPSHOT_LIMIT), use it when constructing params, and after fetching
snapshots compare len(snapshots) to MAX_SNAPSHOT_LIMIT and also compare against
data.get('count') when available; if len(snapshots) == MAX_SNAPSHOT_LIMIT or
data.get('count', 0) > len(snapshots) emit a clear warning via the module logger
so callers know the export may be incomplete (update the code paths around the
params variable and the snapshot-fetching function in telemetry.py).
- Around line 37-40: Telemetry code is using the private client._get() to call
raw HTTP endpoints (e.g., in infrahub_sdk/ctl/telemetry.py building url and
calling client._get), which should be exposed or documented; add a public
accessor on InfrahubClient (e.g., a method named http_get or raw_get that
accepts url, timeout, params and forwards to the underlying HTTP client) and
update telemetry code to call that public method (or alternatively add explicit
public telemetry methods on InfrahubClient such as get_telemetry_snapshots to
encapsulate the URL), and include a short docstring noting intended use for raw
HTTP endpoint access so future callers avoid using _get directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7acedf3-46b5-462e-8e41-679d9478a32a
📒 Files selected for processing (3)
docs/docs/infrahubctl/infrahubctl-telemetry.mdxinfrahub_sdk/ctl/cli_commands.pyinfrahub_sdk/ctl/telemetry.py
2b9f378 to
ac67eb6
Compare
Signed-off-by: Fatih Acar <fatih@opsmill.com>
ac67eb6 to
83edc00
Compare
Summary by CodeRabbit
New Features
Documentation