chore: Switch from OTLP HTTP/protobuf (New Relic) to HTTP/JSON (Grafana) #37404
Conversation
subhrashisdas
requested review from
sharat87,
pratapaprasanna and
KelvinOm
as code owners
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
subhrashisdas
requested review from
rajatagrawal,
nidhi-nair and
ApekshaBhosale
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/src/UITelemetry/auto-otel-web.ts (1)
120-126: Consider adding input validation and cachingThe URL management function is clean but could be enhanced:
- Add input validation for the path parameter
- Consider caching frequently used paths to avoid repeated URL construction
+const urlCache = new Map<string, string>(); + function addPathToCurrentUrl(path: string) { + if (typeof path !== 'string') { + throw new TypeError('Path must be a string'); + } + + const cacheKey = path; + if (urlCache.has(cacheKey)) { + return urlCache.get(cacheKey)!; + } + const origin = window.location.origin; const currentUrl = new URL(origin); currentUrl.pathname = path.startsWith("/") ? path : `/${path}`; - return currentUrl.toString(); + const result = currentUrl.toString(); + urlCache.set(cacheKey, result); + return result; }app/client/package.json (1)
75-84: Consider pinning exact versions for better reproducibility.Using caret (^) in version ranges allows automatic minor version updates which might lead to inconsistent builds. Consider pinning exact versions for better reproducibility in production environments.
- "@opentelemetry/auto-instrumentations-web": "^0.42.0", - "@opentelemetry/context-zone": "^1.27.0", - "@opentelemetry/core": "^1.27.0", - "@opentelemetry/exporter-metrics-otlp-http": "^0.54.2", - "@opentelemetry/exporter-trace-otlp-http": "^0.54.2", - "@opentelemetry/instrumentation": "^0.54.2", - "@opentelemetry/resources": "^1.27.0", - "@opentelemetry/sdk-metrics": "^1.27.0", - "@opentelemetry/sdk-trace-base": "^1.27.0", - "@opentelemetry/sdk-trace-web": "^1.27.0", + "@opentelemetry/auto-instrumentations-web": "0.42.0", + "@opentelemetry/context-zone": "1.27.0", + "@opentelemetry/core": "1.27.0", + "@opentelemetry/exporter-metrics-otlp-http": "0.54.2", + "@opentelemetry/exporter-trace-otlp-http": "0.54.2", + "@opentelemetry/instrumentation": "0.54.2", + "@opentelemetry/resources": "1.27.0", + "@opentelemetry/sdk-metrics": "1.27.0", + "@opentelemetry/sdk-trace-base": "1.27.0", + "@opentelemetry/sdk-trace-web": "1.27.0",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
app/client/package.json(1 hunks)app/client/packages/rts/src/instrumentation.ts(1 hunks)app/client/src/UITelemetry/auto-otel-web.ts(7 hunks)deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs(2 hunks)
🔇 Additional comments (7)
app/client/packages/rts/src/instrumentation.ts (1)
13-13: LGTM on the exporter change.
The switch to HTTP exporter aligns with moving away from protobuf format.
Let's verify all trace exporters are consistently updated:
✅ Verification successful
All trace exporters have been consistently updated to use HTTP format
The search results confirm that:
- No protobuf exporters remain in the codebase
- Both instances of
OTLPTraceExporterinauto-otel-web.tsandinstrumentation.tsuse the HTTP format from@opentelemetry/exporter-trace-otlp-http
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining protobuf exporters
rg "exporter-trace-otlp-proto" -l
# Search for all trace exporters to ensure consistency
rg "OTLPTraceExporter" -A 2
Length of output: 1160
app/client/src/UITelemetry/auto-otel-web.ts (3)
4-4: LGTM: Import changes align with protocol switch
The switch to HTTP exporters (@opentelemetry/exporter-trace-otlp-http and @opentelemetry/exporter-metrics-otlp-http) is correct for the migration to HTTP/JSON format.
Also applies to: 18-18
107-108: Consider environment-specific URL handling
The monitoring URLs are now dynamically constructed based on window.location.origin. Ensure this works correctly across different environments (development, staging, production).
✅ Verification successful
URL handling implementation is correct and environment-safe
The implementation of addPathToCurrentUrl correctly handles URL construction across environments by:
- Using
window.location.originwhich automatically provides the correct protocol, hostname and port - Properly handling path concatenation with leading slashes
- Being used consistently for both traces and metrics endpoints
The current implementation is robust and will work correctly across development, staging, and production environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for environment-specific configurations
rg -g '!node_modules' -g '*.{ts,js,json}' 'window\.location\.origin.*monitoring'
Length of output: 82
Script:
#!/bin/bash
# Let's first check how addPathToCurrentUrl is implemented
rg -g '!node_modules' -g '*.{ts,js}' 'function.*addPathToCurrentUrl' -A 5
# Then check for any environment-specific URL configurations
rg -g '!node_modules' -g '*.{ts,js,json}' -i 'monitoring.*url|monitoring.*endpoint'
# Also check for any environment configuration files
fd -g '*env*.{ts,js,json}'
Length of output: 58816
Script:
#!/bin/bash
# Let's check for any environment-specific URL configurations in environment files
rg -g '!node_modules' -g '*.{ts,js}' 'monitoring.*url|monitoring.*endpoint' app/client/src/ce/actions/environmentAction.ts app/client/src/ee/actions/environmentAction.ts
# Check for any environment-specific configurations in the monitoring setup
rg -g '!node_modules' -g '*.{ts,js}' 'addPathToCurrentUrl' -A 10 -B 10
Length of output: 5377
47-47: Verify Grafana endpoint compatibility
The new monitoring endpoints ("/monitoring/traces" and "/monitoring/metrics") should be verified against Grafana's requirements.
Also applies to: 77-77
✅ Verification successful
The previous search was too specific. Let's broaden our search to understand the monitoring setup and verify the endpoint compatibility. We'll look for telemetry configurations, route definitions, and OpenTelemetry exporters to verify if these endpoints are properly configured.
Let's verify how these monitoring endpoints are handled on the server side and examine the complete trace exporter configuration to ensure the endpoints are properly set up.
The monitoring endpoints are correctly configured for OpenTelemetry
The /monitoring/traces endpoint is properly configured in both client and server implementations:
- Server-side uses a dedicated New Relic OTLP endpoint for traces
- Client-side implementation correctly uses the local monitoring endpoint with proper compression and authentication headers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any Grafana configuration files or documentation
rg -g '!node_modules' -g '*.{md,txt,yaml,yml,json}' -i 'grafana.*endpoint|monitoring/(traces|metrics)'
Length of output: 104
Script:
#!/bin/bash
# Let's search for any monitoring or telemetry configuration files
fd -e yaml -e yml -e json -e ts -e js "telemetry|monitoring|grafana" --exec rg -l "endpoint|traces|metrics|monitoring"
# Search for any route definitions or server configurations
rg -g '*.{ts,js}' "app.(use|get|post)\(.*monitoring.*\)"
# Look for any OpenTelemetry configuration
rg -g '*.{ts,js}' "OTLPTraceExporter|opentelemetry.*exporter"
Length of output: 773
Script:
#!/bin/bash
# Let's examine the server-side route handling for these endpoints
rg -g '*.{ts,js}' -B 5 -A 5 "'/monitoring/(traces|metrics)'"
# Check the actual implementation of the trace exporter configuration
ast-grep --pattern 'new OTLPTraceExporter({
$$$
})'
Length of output: 1169
deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs (2)
41-47: LGTM! Clean and well-structured monitoring configuration.
The monitoring endpoints configuration follows good practices with clear path mappings.
153-153: 🛠️ Refactor suggestion
Verify API key environment variable.
Add a validation check for the required environment variable APPSMITH_NEW_RELIC_OTLP_LICENSE_KEY.
Consider adding environment variable validation at the start of the script:
if (!process.env.APPSMITH_NEW_RELIC_OTLP_LICENSE_KEY) {
console.error("Missing required environment variable: APPSMITH_NEW_RELIC_OTLP_LICENSE_KEY");
process.exit(1);
}app/client/package.json (1)
75-84: OpenTelemetry dependencies updated for HTTP/JSON support.
The updates align with the PR objective of switching from OTLP HTTP/protobuf to HTTP/JSON format.
Let's verify these versions for any known vulnerabilities:
✅ Verification successful
Let me verify if these versions are compatible with HTTP/JSON format by checking the codebase for any OpenTelemetry configuration.
OpenTelemetry dependency updates are safe and compatible
The updated versions are compatible with HTTP/JSON format and show no security vulnerabilities. The codebase already uses OTLP HTTP exporters with compression and proper configuration for both traces and metrics.
- No security advisories found for the OpenTelemetry packages
- Existing configuration in
auto-otel-web.tsusesOTLPTraceExporterandOTLPMetricExporterwith GZIP compression - Version updates are minor and maintain backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories for OpenTelemetry packages
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NPM, package: "opentelemetry") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 347
Script:
#!/bin/bash
# Search for OpenTelemetry configuration files and usage
rg -l "opentelemetry" --type ts --type js
# Look for OTLP exporter configuration
rg -A 5 "OTLPTrace|OTLPMetric" --type ts --type js
Length of output: 3687
subhrashisdas
force-pushed
the
feature/otlp-web-ce
branch
from
9b54ae0
to
2fc7d29
Compare
subhrashisdas
changed the title
github-actions
bot
added
the
skip-changelog
subhrashisdas
force-pushed
the
feature/otlp-web-ce
branch
from
2fc7d29
to
6f60f61
Compare
subhrashisdas
force-pushed
the
feature/otlp-web-ce
branch
from
6f60f61
to
065cddc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/UITelemetry/auto-otel-web.ts (1)
120-129: Add input validation and error handlingWhile the URL management function works correctly, it could benefit from some defensive programming.
Consider this enhanced implementation:
function addPathToCurrentUrl(path: string) { + if (!path) { + throw new Error("Path parameter is required"); + } const origin = window.location.origin; + if (!origin) { + throw new Error("Unable to determine current origin"); + } const currentUrl = new URL(origin); currentUrl.pathname = path.startsWith("/") ? path : `/${path}`; return currentUrl.toString(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
app/client/package.json(1 hunks)app/client/packages/rts/src/instrumentation.ts(2 hunks)app/client/src/UITelemetry/auto-otel-web.ts(7 hunks)deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/package.json
- app/client/packages/rts/src/instrumentation.ts
- deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs
🔇 Additional comments (3)
app/client/src/UITelemetry/auto-otel-web.ts (3)
4-4: LGTM: Import changes align with HTTP/JSON migration
The switch from protobuf to HTTP exporters is correctly implemented.
Also applies to: 18-18
107-108: LGTM: Properly configured to prevent telemetry loops
The monitoring endpoints are correctly added to the ignore list to prevent self-monitoring loops.
47-50: Verify monitoring endpoints configuration
The exporter configurations look correct, but we should verify that the /monitoring/traces and /monitoring/metrics endpoints are properly configured to handle the requests.
#!/bin/bash
# Search for monitoring endpoint configurations in infrastructure files
rg -g '!node_modules' -g '*.{yaml,yml,conf,json}' '/monitoring/(traces|metrics)'Also applies to: 77-81
|
@subhrashisdas What do you think about updating the RTS dependencies too? |
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 065cddc yet
Fri, 15 Nov 2024 11:48:13 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
/monitoring/tracesand/monitoring/metrics.Bug Fixes
Chores