Add instrumentation support for ballerina#1172
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR adds Ballerina support to agent creation, trait wiring, and Helm templates. It introduces a Ballerina buildpack path in the UI and payload, Ballerina-specific trait parameters in the client, and Ballerina OTEL instrumentation handling in deployment templates. ChangesBallerina agent instrumentation support
Sequence Diagram(s)sequenceDiagram
participant InternalAgentForm
participant buildAgentCreationPayload
participant AgentManager
participant openchoreosvc client
participant Helm trait config
InternalAgentForm->>buildAgentCreationPayload: submit Ballerina agent build payload
buildAgentCreationPayload->>AgentManager: create agent request
AgentManager->>openchoreosvc client: build Ballerina trait parameters
AgentManager->>Helm trait config: apply instrumentation settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
console/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsx (1)
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated import snippet into a constant.
"import ballerinax/amp as _;"is hard-coded both inhandleCopyBalImport(line 92) and in the rendered<pre>(line 549). If one is edited the copy button silently diverges from the displayed text. Define it once and reuse.Also applies to: 549-549
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsx` at line 92, The AMP import snippet is duplicated between handleCopyBalImport and the rendered <pre>, so the copy action can drift from what is displayed. Extract the shared string into a single constant in InternalAgentForm and use it both in the clipboard writeText call and in the UI rendering to keep them in sync.deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/bal-otel-instrumentation-trait.yaml (1)
70-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm
configMountDirandBAL_CONFIG_FILESrelative-path coupling.The toml is mounted at
${parameters.configMountDir}/am-instrumentation.toml, butBAL_CONFIG_FILESreferences it by the relative nameam-instrumentation.toml, which Ballerina resolves against its WORKDIR. If an operator overridesconfigMountDirto a directory other than the run-image WORKDIR (/workspace), Ballerina will not find the file. The inline comment notes this, but the two values can silently drift. Consider deriving the env value fromconfigMountDirto keep them in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/bal-otel-instrumentation-trait.yaml` around lines 70 - 78, The `bal-otel-instrumentation-trait.yaml` mount path and `BAL_CONFIG_FILES` value are coupled but currently defined independently, so overriding `configMountDir` can break Ballerina’s lookup. Update the trait so the env var is derived from the same `configMountDir` used in the `volumeMounts` entry for `am-instrumentation.toml`, keeping `BAL_CONFIG_FILES` in sync with the mounted location. Use the existing `configMountDir`, `BAL_CONFIG_FILES`, and `am-instrumentation.toml` symbols to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent-manager-service/clients/openchoreosvc/client/constants.go`:
- Around line 113-116: Add the new Ballerina config-var constants to
SystemInjectedEnvVars so the agent treats them as internal injected values.
Update the env-var filter/read-only handling in the code path used by
agent_manager.go, referencing BalConfigVarOTELEndpoint and
BalConfigVarAgentAPIKey, and make sure both
BAL_CONFIG_VAR_BALLERINAX_AMP_OTELENDPOINT and
BAL_CONFIG_VAR_BALLERINAX_AMP_APIKEY are included alongside the existing
injected env vars.
In `@console/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsx`:
- Around line 523-533: handleFieldChange currently clears conditional validation
only for python and docker, so stale errors can survive when language switches
to Ballerina. Update the language-dependent error reset logic in
InternalAgentForm’s handleFieldChange to add a ballerina branch that clears any
no-longer-relevant errors from the hidden Python/Docker fields, using the
existing field/error handling pattern already used for python and docker.
In
`@deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/bal-otel-instrumentation-trait.yaml`:
- Around line 80-85: The BAL_CONFIG_FILES value in the
bal-otel-instrumentation-trait template is hard-coded to include Config.toml,
which can break workloads that do not ship that file. Update the logic around
the BAL_CONFIG_FILES env entry in the component trait template to make the
Config.toml portion conditional based on its presence, or clearly
enforce/document that Config.toml is required for this trait; use the existing
BAL_CONFIG_FILES/env injection block to locate the change.
---
Nitpick comments:
In `@console/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsx`:
- Line 92: The AMP import snippet is duplicated between handleCopyBalImport and
the rendered <pre>, so the copy action can drift from what is displayed. Extract
the shared string into a single constant in InternalAgentForm and use it both in
the clipboard writeText call and in the UI rendering to keep them in sync.
In
`@deployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/bal-otel-instrumentation-trait.yaml`:
- Around line 70-78: The `bal-otel-instrumentation-trait.yaml` mount path and
`BAL_CONFIG_FILES` value are coupled but currently defined independently, so
overriding `configMountDir` can break Ballerina’s lookup. Update the trait so
the env var is derived from the same `configMountDir` used in the `volumeMounts`
entry for `am-instrumentation.toml`, keeping `BAL_CONFIG_FILES` in sync with the
mounted location. Use the existing `configMountDir`, `BAL_CONFIG_FILES`, and
`am-instrumentation.toml` symbols to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c908606-aada-4268-87f8-9b5a0e0a858c
📒 Files selected for processing (9)
agent-manager-service/clients/openchoreosvc/client/components.goagent-manager-service/clients/openchoreosvc/client/constants.goagent-manager-service/services/agent_manager.goconsole/workspaces/libs/views/src/icons/BuildpackIcon.tsxconsole/workspaces/pages/add-new-agent/src/forms/InternalAgentForm.tsxconsole/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.tsdeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/bal-otel-instrumentation-trait.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-traits/instrumentation-trait-env-injection.yamldeployments/helm-charts/wso2-amp-platform-resources-extension/templates/component-types/agent-api.yaml
hanzjk
force-pushed
the
trace-key
branch
3 times, most recently
from
bdfe46e to
ae6f061
Compare
2 participants
Purpose
Resolves #1110
Introduced a new trait to mount an
am-instrumentation.tomlfile with the following content and set the config file path using theBAL_CONFIG_FILESenvironment variable:This is done using an init container with a BusyBox image.
Also updated the existing trait that injects the OTEL endpoint and API key to support renaming the environment variables that are injected with those values. The default environment variable names are
AMP_OTEL_ENDPOINTandAMP_AGENT_API_KEY.This configuration is included by default in the
Ballerina.tomlfile generated when initiating a new package using thebal newcommand in Ballerina.Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit