[ROB-1923] - amp irsa support #465
Conversation
WalkthroughDependency versions were updated in pyproject.toml and requirements.txt. Prometheus AWS configuration gained optional AWS profile support and passes through session tokens. A Prometheus metrics service warning log now includes the result count and query string. No other control flow or packaging structure changes were made. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Caller
participant Settings as Settings
participant Utils as PrometheusUtils
participant B3 as boto3.Session
participant APS as AWSPrometheusConfig
participant PC as PrometheusClient
User->>Utils: build_prometheus_config(Settings)
alt AWS-managed Prometheus
Utils->>B3: create Session(profile_name?)
Note right of B3: Uses default session if no profile provided
B3-->>Utils: credentials (access_key, secret_key, token?)
Utils->>APS: new AWSPrometheusConfig(region, service_name, access_key?, secret_key?, token)
APS-->>Utils: config
else Other backends (Coralogix/Victoria/Default)
Utils-->>User: corresponding config
end
Utils->>PC: instantiate client with config
PC-->>User: client ready
note over APS,PC: Token is forwarded when present
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyproject.toml (1)
25-44: Add missingboto3dependency to PoetryThe import scan confirms that
robusta_krr/core/integrations/prometheus/prometheus_utils.pyusesimport boto3(line 5), butpyproject.toml’s[tool.poetry.dependencies]section does not listboto3, which will cause Poetry-based installations to fail.• robusta_krr/core/integrations/prometheus/prometheus_utils.py:
– line 5:import boto3
• pyproject.toml –[tool.poetry.dependencies]missingboto3Suggested patch:
[tool.poetry.dependencies] python = ">=3.9,<=3.12.9" typer = { extras = ["all"], version = "^0.7.0" } pydantic = "^1.10.7" kubernetes = "^26.1.0" prometheus-api-client = "0.5.3" numpy = ">=1.26.4,<1.27.0" alive-progress = "^3.1.2" + boto3 = "^1.34.62" slack-sdk = "^3.21.3" pandas = "2.2.2" requests = "2.32.0" pyyaml = "6.0.1" typing-extensions = "4.6.0" idna = "3.7" urllib3 = "^1.26.20" setuptools = "^80.9.0" zipp = "^3.19.1" tenacity = "^9.0.0"robusta_krr/core/integrations/prometheus/prometheus_utils.py (1)
41-55: Guard against missing AWS credentials in default session
session.get_credentials()can return None. Dereferencing it (freezing, accessing fields) will raise. Add a check and raise a clear, actionable error that hints at IRSA or environment setup.- credentials = session.get_credentials() + credentials = session.get_credentials() region = settings.eks_managed_prom_region if settings.eks_managed_prom_region else session.region_name token = None if settings.eks_access_key and settings.eks_secret_key: @@ - else: + else: # we need at least one parameter from credentials, but we should use whatever we can from settings (this has higher precedence) - credentials = credentials.get_frozen_credentials() + if credentials is None: + raise RuntimeError( + "No AWS credentials found. Provide eks_access_key/eks_secret_key or configure the environment/IRSA/profile." + ) + credentials = credentials.get_frozen_credentials() access_key = settings.eks_access_key if settings.eks_access_key else credentials.access_key secret_key = settings.eks_secret_key.get_secret_value() if settings.eks_secret_key else credentials.secret_key token = credentials.token
🧹 Nitpick comments (4)
robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py (1)
127-133: Use total_seconds() when formatting PromQL step to avoid day-truncation
timedelta.secondsignores days. If we ever pass a step ≥ 24h, it will be truncated. Useint(step.total_seconds())instead.- lambda: self.prometheus.safe_custom_query_range( - query=query, start_time=start, end_time=end, step=f"{step.seconds}s" - )["result"], + lambda: self.prometheus.safe_custom_query_range( + query=query, + start_time=start, + end_time=end, + step=f"{int(step.total_seconds())}s", + )["result"],robusta_krr/core/integrations/prometheus/prometheus_utils.py (3)
37-41: AWS profile support — good addition; consider helpful error for bad profileSelecting a session via
profile_nameis great. If the named profile is missing, boto3 raisesProfileNotFound. Consider catching that to output a clearer message that mentions the setting name.Example:
- session = boto3.Session(profile_name=settings.eks_managed_prom_profile_name) + try: + session = boto3.Session(profile_name=settings.eks_managed_prom_profile_name) + except Exception as e: + # boto3.exceptions.ProfileNotFound or similar + raise RuntimeError( + f"EKS Managed Prometheus profile '{settings.eks_managed_prom_profile_name}' not found" + ) from e
45-55: Optional: allow explicit session token in settings (if available)If the settings model exposes a session token (e.g.,
eks_session_token), prefer it overcredentials.tokento support explicitly-provided temporary creds. If not present, ignore.
56-56: Service name selection may be surprising
service_name = settings.eks_service_name if settings.eks_secret_key else "aps"ties the choice to whether a secret key was provided. If the intent is “use provided override when set, else default to 'aps'”, consider:- service_name = settings.eks_service_name if settings.eks_secret_key else "aps" + service_name = settings.eks_service_name or "aps"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.toml(1 hunks)requirements.txt(3 hunks)robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py(1 hunks)robusta_krr/core/integrations/prometheus/prometheus_utils.py(3 hunks)
🔇 Additional comments (4)
robusta_krr/core/integrations/prometheus/metrics_service/prometheus_metrics_service.py (1)
216-221: More actionable warning — good improvementIncluding the actual series count and the original query in the warning will make triage much easier when queries accidentally return 0 or multiple series. LGTM.
pyproject.toml (1)
33-33: Alignprometrixto version 0.2.2a3 across all manifestsVerified via direct inspection that the
AWSPrometheusConfigmodel in both 0.2.2a2 and 0.2.2a3 includes atokenfield, so upgrading is safe and avoids drift:
- pyproject.toml (line 33):
- prometrix = {version = "0.2.2a2", allow-prereleases = true} + prometrix = {version = "0.2.2a3", allow-prereleases = true}- Regenerate your requirements.txt to match Poetry’s lockfile:
(or runpoetry export -f requirements.txt --without-hashes -o requirements.txtpip-compileif you’re using pip-tools)This alignment ensures that both build paths use the same
prometrixversion and prevents any “unexpected keyword argument” errors when passingtokentoAWSPrometheusConfig.requirements.txt (1)
49-50: Confirm shellingham pin or intentional omissionTyper’s
allextra for version 0.7.0 includes bothrichandshellinghamfor nicely formatted errors and automatic shell detection, respectively (pypi.org). Yourrequirements.txtalready pinsrichand the transitivecolorama, but does not includeshellingham.Please choose one:
If you need automatic shell detection for
--install-completion, add under the same Python markers:tenacity==9.0.0 ; python_version >= "3.9" and python_full_version < "3.13" typer==0.7.0 ; python_version >= "3.9" and python_full_version < "3.13" + shellingham==1.5.4 ; python_version >= "3.9" and python_full_version < "3.13"Otherwise, confirm that omitting
shellinghamis intentional and that you won’t need the--install-completionUX.robusta_krr/core/integrations/prometheus/prometheus_utils.py (1)
60-67: EnsureprometrixDependency Supports thetokenKwargForwarding the session token for IRSA/temporary credentials is correct, but before merging:
- Verify that the
prometrixpackage version you’ve pinned (in bothpyproject.tomlandrequirements.txt) actually supports passing atokenparameter toAWSPrometheusConfig.- If you’re unsure which version introduced this feature, check the
prometrixchangelog or run:orpip show prometrixand confirm the minimum version that includes thepoetry show prometrixtokenargument.- Update your dependency constraint accordingly (e.g.
prometrix>=<min_version>) to prevent runtime failures if older releases lack this keyword.
arikalon1
left a comment
There was a problem hiding this comment.
nice work
some minor comments
No description provided.