Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request introduces OpenTelemetry observability infrastructure across the application. Changes include adding initialization code in Django and ASGI entry points, configuring Docker Compose services with observability networks, updating environment configuration examples, and updating the shared dependency reference to enable telemetry features. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Code ReviewThanks for the PR! The health check fix is correct, but there are several concerns worth addressing — especially one that could break reproducible builds. Critical: Mutable branch reference in
|
| Issue | Severity |
|---|---|
Branch pin (@openobserve) instead of tag/SHA |
High — breaks reproducible builds |
| PR mixes unrelated changes | Medium |
| OTel graceful degradation with missing endpoint | Medium |
| No tests for OTel integration | Medium |
example.env default endpoint |
Low |
Missing comments in asgi.py and health check |
Low |
The health check fix is good to go once it's separated. The OTel work needs the dependency pin resolved before merge.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates OpenTelemetry for application-wide observability, adding OTLP exporter configurations, environment variables, and initialization logic in manage.py and asgi.py. It also updates the adit-radis-shared dependency to a branch reference and switches the production healthcheck to HTTPS. Feedback focuses on ensuring build stability by using immutable version tags instead of branch references for dependencies. Additionally, it is recommended to guard telemetry initialization with an activity check to prevent unnecessary overhead in environments where observability is not enabled.
pyproject.toml
Outdated
| requires-python = ">=3.12,<4.0" | ||
| dependencies = [ | ||
| "adit-radis-shared @ git+https://github.com/openradx/adit-radis-shared.git@0.19.1", | ||
| "adit-radis-shared @ git+https://github.com/openradx/adit-radis-shared.git@openobserve", |
There was a problem hiding this comment.
Using a branch reference (@openobserve) for a dependency is discouraged for production-bound code. Branches are mutable, which can lead to non-deterministic builds if the branch is updated. It is safer to use a specific version tag or a commit hash to ensure environment stability.
manage.py
Outdated
| from adit_radis_shared.telemetry import setup_opentelemetry | ||
|
|
||
| setup_opentelemetry() |
There was a problem hiding this comment.
OpenTelemetry is initialized unconditionally in manage.py, which affects all management commands (including migrations and shell). It is recommended to guard this with is_telemetry_active() to avoid unnecessary overhead or connection attempts in environments where telemetry is not required, consistent with the logic used in the settings.
| from adit_radis_shared.telemetry import setup_opentelemetry | |
| setup_opentelemetry() | |
| from adit_radis_shared.telemetry import setup_opentelemetry, is_telemetry_active | |
| if is_telemetry_active(): | |
| setup_opentelemetry() |
radis/asgi.py
Outdated
| from adit_radis_shared.telemetry import setup_opentelemetry # noqa: E402 | ||
|
|
||
| setup_opentelemetry() |
There was a problem hiding this comment.
OpenTelemetry is initialized unconditionally here. Consider guarding this call with is_telemetry_active() to ensure it only runs when telemetry is explicitly configured, consistent with the logic used in radis/settings/base.py.
| from adit_radis_shared.telemetry import setup_opentelemetry # noqa: E402 | |
| setup_opentelemetry() | |
| from adit_radis_shared.telemetry import setup_opentelemetry, is_telemetry_active # noqa: E402 | |
| if is_telemetry_active(): | |
| setup_opentelemetry() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docker-compose.override.yml.example (1)
9-11: Ensure documentation covers the external network prerequisite.The
openradx-observabilitynetwork must exist before using this override (created by the referenced observability stack). Consider adding a brief comment in this file noting this dependency, or ensure the linked documentation inexample.envcovers this clearly.📝 Suggested comment addition
networks: openradx-observability: + # This network is created by the openradx-observability stack. + # See https://github.com/openradx/openradx-observability external: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.override.yml.example` around lines 9 - 11, Add a short comment above the networks block in docker-compose.override.yml.example that explains the external network "openradx-observability" must be created beforehand (it’s provided by the observability stack), and point readers to the example.env or observability stack documentation for steps to create it so users know this prerequisite before using the override.pyproject.toml (1)
10-10: Using a branch reference withuv.lockpinning is reproducible but not ideal practice.The
uv.lockfile pinsadit-radis-sharedto a specific commit (a945c9d93841fc6c376df7aec78550fec1715b93) on theopenobservebranch, so builds remain reproducible. However, relying on a branch reference inpyproject.tomlwhile pinning in the lock file is inconsistent with best practices.For clarity and proper version communication, tag and release the
openobservebranch changes as a stable version inadit-radis-shared, then update this reference to use the version tag instead of the branch name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 10, The pyproject dependency string "adit-radis-shared @ git+https://github.com/openradx/adit-radis-shared.git@openobserve" should not point to a branch; instead tag and release the current openobserve commit in the adit-radis-shared repo (create a semantic version tag), update the pyproject.toml dependency to use that version tag (e.g., replace `@openobserve` with `@vX.Y.Z`), and then regenerate the lockfile (uv.lock) so it pins the released tag commit consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docker-compose.override.yml.example`:
- Around line 9-11: Add a short comment above the networks block in
docker-compose.override.yml.example that explains the external network
"openradx-observability" must be created beforehand (it’s provided by the
observability stack), and point readers to the example.env or observability
stack documentation for steps to create it so users know this prerequisite
before using the override.
In `@pyproject.toml`:
- Line 10: The pyproject dependency string "adit-radis-shared @
git+https://github.com/openradx/adit-radis-shared.git@openobserve" should not
point to a branch; instead tag and release the current openobserve commit in the
adit-radis-shared repo (create a semantic version tag), update the
pyproject.toml dependency to use that version tag (e.g., replace `@openobserve`
with `@vX.Y.Z`), and then regenerate the lockfile (uv.lock) so it pins the
released tag commit consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2def08d-fa36-4456-879c-3c2fd49c8948
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.gitignoredocker-compose.override.yml.exampledocker-compose.prod.ymlexample.envmanage.pypyproject.tomlradis/asgi.pyradis/settings/base.py
The health check was using plain HTTP, but SECURE_SSL_REDIRECT causes a 301 redirect to HTTPS, making the probe fail and Swarm restart replicas. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
samuelvkwong
force-pushed
the
fix-healthcheck-https
branch
from
ef444cf to
044e7dc
Compare
1 participant
Summary
curl -f http://localhost/health/, butSECURE_SSL_REDIRECTreturns a 301 redirect to HTTPSmax_attemptswas exhaustedcurl -fk https://localhost/health/to hit the HTTPS endpoint directly (-kskips cert verification for localhost)Test plan
curl -fk https://localhost/health/returns{"status": "ok"}docker psshows containers as(healthy)docker service ps🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores