chore: remove metrics in fe #38240
chore: remove metrics in fe #38240
Conversation
WalkthroughThe pull request simplifies the telemetry setup in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/UITelemetry/auto-otel-web.ts (1)
83-89: Consider adding input validationThe function implementation is clean and well-documented. However, consider adding validation for the path parameter to handle edge cases.
function getAbsoluteUrl(path: string) { + if (!path.startsWith('/')) { + path = '/' + path; + } return new URL(path, window.location.origin).toString(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/UITelemetry/auto-otel-web.ts(3 hunks)
🔇 Additional comments (2)
app/client/src/UITelemetry/auto-otel-web.ts (2)
71-72: LGTM! Consistent URL handling
The ignoreResourceUrls configuration correctly uses the new getAbsoluteUrl method.
Line range hint 38-42: LGTM! Verify trace endpoint accessibility
The trace exporter configuration looks correct with proper compression and security headers.
✅ Verification successful
Let me check the actual usage context of this endpoint to ensure it's consistently configured across the codebase.
Trace endpoint configuration is properly implemented
The endpoint /monitoring/traces is correctly configured:
- Used as the OTLP exporter URL with proper headers and compression
- Properly excluded from PageLoadInstrumentation tracking
- Correctly mapped in Caddy configuration to
/v1/traces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the monitoring endpoint is consistently referenced
rg -l "/monitoring/traces"
Length of output: 122
Script:
#!/bin/bash
# Check the content of files referencing the monitoring endpoint
rg -C 5 "/monitoring/traces"
Length of output: 2389
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/UITelemetry/auto-otel-web.ts (1)
83-89: Consider adding input validationWhile the URL construction is robust, consider adding validation for the path parameter to handle edge cases.
function getAbsoluteUrl(path: string) { + if (!path) { + throw new Error("Path parameter is required"); + } + if (!path.startsWith("/")) { + path = `/${path}`; + } return new URL(path, window.location.origin).toString(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/UITelemetry/auto-otel-web.ts(3 hunks)
🔇 Additional comments (3)
app/client/src/UITelemetry/auto-otel-web.ts (3)
71-72: LGTM! URL filtering properly updated
The ignore list correctly uses the new URL construction method while maintaining all necessary filtering.
Line range hint 1-89: Verify complete removal of metrics configuration
The metrics removal appears complete, but let's verify no remnants remain.
✅ Verification successful
Let me search for any remaining metrics-related imports and configurations.
Let me check for any remaining metrics-related code using a broader search pattern.
Let me try one final check with the correct package.json path.
Let me check the specific metrics usage in the PageLoadInstrumentation file.
Metrics removal is complete and verified
The remaining references to "metric" in PageLoadInstrumentation.ts are only for web vitals types (LCPMetricWithAttribution, FCPMetricWithAttribution) and don't involve OpenTelemetry metrics collection. No metrics-related imports, configurations, or instrumentation code remains in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining metrics-related code
rg -i "metric|meter" --type ts
Length of output: 66911
Script:
#!/bin/bash
# Check for metrics-related imports and configurations in the telemetry file
ast-grep --pattern 'import { $_ } from "@opentelemetry/metrics"' app/client/src/UITelemetry/auto-otel-web.ts
# Check for metrics-related configurations
ast-grep --pattern 'const $_ = new MeterProvider' app/client/src/UITelemetry/auto-otel-web.ts
Length of output: 206
Script:
#!/bin/bash
# Search for metrics-related code in the telemetry file
rg -i "meter|metric|measurement" app/client/src/UITelemetry/auto-otel-web.ts
# Check for any remaining metrics packages in package.json
rg '"@opentelemetry/metrics"' package.json
Length of output: 176
Script:
#!/bin/bash
# Check for any remaining metrics packages in app package.json
rg '"@opentelemetry/metrics"' app/client/package.json
# Look for any metrics configuration or setup code
rg -l "MeterProvider|MetricReader|Metric" app/client/src/UITelemetry/
Length of output: 181
Script:
#!/bin/bash
# Check the content of PageLoadInstrumentation.ts for metrics usage
cat app/client/src/UITelemetry/PageLoadInstrumentation.ts
Length of output: 10970
Line range hint 38-42: LGTM! Verify trace endpoint accessibility
The trace exporter configuration is correct with proper security headers and compression.
✅ Verification successful
Let me check the usage of getAbsoluteUrl to ensure it's properly implemented.
Let me try a different search approach to find the implementation of getAbsoluteUrl.
URL construction is properly implemented
The getAbsoluteUrl function is correctly implemented in the same file using the standard URL constructor with window.location.origin, ensuring proper URL resolution. The function is used consistently for the trace endpoint and is properly excluded from monitoring.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify no hardcoded URLs remain in the codebase
rg -g '*.{ts,js}' '/monitoring/traces' --no-filename
Length of output: 143
Script:
#!/bin/bash
# Find the implementation of getAbsoluteUrl
ast-grep --pattern 'function getAbsoluteUrl'
# Also check for any const/let declarations
ast-grep --pattern 'const getAbsoluteUrl ='
ast-grep --pattern 'let getAbsoluteUrl ='
# Backup search with ripgrep for any other definitions
rg 'getAbsoluteUrl.*=' -A 3
Length of output: 76711
Script:
#!/bin/bash
# Search for getAbsoluteUrl function or import
rg -g '*.{ts,js}' -B 2 -A 2 'getAbsoluteUrl'
# Also check if it's imported from another module
rg -g '*.{ts,js}' 'export.*getAbsoluteUrl'
Length of output: 1182
Description
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="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12396635756
Commit: 15b6d54
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 18 Dec 2024 16:41:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes