chore: Distributed tracing for client #37101
Conversation
WalkthroughThe changes in this pull request involve updates to the application's telemetry capabilities. Key modifications include the addition of a new dependency for OpenTelemetry instrumentation, updates to existing dependencies, and enhancements to the configuration for observability. The telemetry service's metadata structure has been updated, and several properties have been added or removed to streamline the configuration. Overall, these changes aim to improve the application's telemetry and observability features. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (2)
app/client/src/ce/configs/index.ts (1)
98-101: Consider environment-specific deployment names.While the defaults look good, consider using different default deployment names for different environments (dev/staging/prod).
- deploymentName: process.env.APPSMITH_DEPLOYMENT_NAME || "self-hosted", + deploymentName: process.env.APPSMITH_DEPLOYMENT_NAME || + `self-hosted-${process.env.NODE_ENV || 'development'}`,app/client/package.json (1)
Line range hint
75-85: Maintain consistent versioning across OpenTelemetry packages.Consider pinning all OpenTelemetry packages to specific versions to prevent compatibility issues. Currently mixing fixed versions (1.25.1, 0.52.1) with floating versions (^0.41.0, ^1.27.0).
Apply this diff to maintain consistency:
- "@opentelemetry/auto-instrumentations-web": "^0.41.0", + "@opentelemetry/auto-instrumentations-web": "0.41.0", - "@opentelemetry/semantic-conventions": "^1.27.0", + "@opentelemetry/semantic-conventions": "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 (2 hunks)
- app/client/src/UITelemetry/auto-otel-web.ts (5 hunks)
- app/client/src/ce/configs/index.ts (4 hunks)
- app/client/src/ce/configs/types.ts (1 hunks)
🔇 Additional comments (10)
app/client/src/ce/configs/types.ts (2)
23-27: LGTM: Well-structured observability configuration.The new observability configuration follows OpenTelemetry semantic conventions for resource attributes.
23-27: Verify NewRelic and OpenTelemetry configuration relationship.Since both NewRelic and OpenTelemetry configurations coexist, let's verify their integration in the codebase.
Also applies to: 28-37
✅ Verification successful
NewRelic and OpenTelemetry configurations are properly integrated
The configurations are correctly implemented with NewRelic acting as the OTLP endpoint for OpenTelemetry:
- OpenTelemetry is configured to send traces and metrics to NewRelic's OTLP endpoint
- Observability fields (deploymentName, serviceInstanceId, serviceName) are properly utilized in the telemetry setup
- Configuration values are appropriately sourced from environment variables with fallbacks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of both configurations to ensure proper integration echo "Searching for NewRelic OTLP usage..." rg -l "otlpEndpoint|otlpLicenseKey" app/client/src echo -e "\nSearching for observability config usage..." rg -l "observability.*deploymentName|observability.*serviceInstanceId|observability.*serviceName" app/client/srcLength of output: 502
Script:
#!/bin/bash # Let's examine the implementation details of these configurations echo "Checking NewRelic OTLP implementation..." rg -A 10 "otlpEndpoint|otlpLicenseKey" app/client/src/UITelemetry/auto-otel-web.ts echo -e "\nChecking observability config usage..." rg -A 10 "observability" app/client/src/ce/configs/index.ts echo -e "\nChecking for any OpenTelemetry initialization..." rg "OpenTelemetry|otel" app/client/src/UITelemetry/Length of output: 3852
app/client/src/UITelemetry/auto-otel-web.ts (4)
41-43: LGTM! Resource attributes properly configuredThe resource attributes are correctly configured using the new semantic conventions.
87-89: LGTM! Consistent resource attributionResource attributes are consistently applied across both tracer and meter providers.
73-73: LGTM! Standard W3C propagator is a better choiceUsing the standard W3CTraceContextPropagator improves interoperability.
#!/bin/bash # Verify if custom propagator is completely removed rg "CustomW3CTraceContextPropagator" --type ts
7-10: Consider stability implications of incubating conventionsThe semantic conventions are imported from the incubating package which may be subject to breaking changes in future releases.
Also applies to: 30-33
app/client/src/ce/configs/index.ts (3)
16-19: LGTM: Clean interface addition for observability config.The new observability interface is well-structured and follows the existing pattern.
179-186: LGTM: Config processing follows established patterns.The implementation of observability config processing is consistent with the existing codebase patterns.
283-286: 🛠️ Refactor suggestionReview hardcoded serviceName value.
The
serviceName: "frontend"is hardcoded. Consider making it configurable through environment variables for consistency with other properties.- serviceName: "frontend", + serviceName: process.env.APPSMITH_SERVICE_NAME || "frontend",app/client/package.json (1)
75-75: Consider security implications of auto-instrumentation.The new dependency
@opentelemetry/auto-instrumentations-webautomatically instruments all web requests. Ensure this aligns with your security requirements and doesn't expose sensitive data in traces.✅ Verification successful
Let me search for OpenTelemetry configuration and setup to better understand how auto-instrumentation is being used.
Auto-instrumentation is properly configured with security considerations
The implementation in
UITelemetry/auto-otel-web.tsshows:
- Explicit resource URLs are ignored in instrumentation configuration
- Custom PageLoadInstrumentation is used instead of default auto-instrumentation
- Proper configuration of tracer provider with deployment context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing security policies or configurations for tracing rg -l "opentelemetry|OTEL|telemetry" | grep -i "security|config|environment"Length of output: 78
Script:
#!/bin/bash # Search for OpenTelemetry configuration files and settings rg -l "opentelemetry|OTEL|telemetry" --type js --type ts --type json # Look for environment variables or configuration related to telemetry rg "OTEL_|opentelemetry" .env* *config* 2>/dev/null # Check for any tracing or instrumentation setup code rg -A 5 "new TracerProvider|instrumentations:|WebTracerProvider" --type js --type tsLength of output: 3364
| getWebAutoInstrumentations({ | ||
| "@opentelemetry/instrumentation-xml-http-request": { | ||
| enabled: true, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider additional XHR instrumentation configuration
While the basic setup is good, consider enhancing the XHR instrumentation with:
- URL ignore patterns (similar to PageLoadInstrumentation)
- Custom headers propagation rules
- Error handling configuration
Example configuration:
getWebAutoInstrumentations({
"@opentelemetry/instrumentation-xml-http-request": {
enabled: true,
+ ignoreUrls: [browserAgentEndpoint, otlpEndpoint, smartlookBaseDomain],
+ propagateTraceHeaderCorsUrls: /.*/, // Configure URLs where trace headers should be propagated
+ clearTimingResources: true,
},
}),Committable suggestion was skipped due to low confidence.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11517104531. |
|
Deploy-Preview-URL: https://ce-37101.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11521229053. |
|
Deploy-Preview-URL: https://ce-37101.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11521577149. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/public/index.html (1)
231-234: Consider documenting the observability configuration.The observability configuration looks good, but would benefit from a brief comment explaining:
- The purpose of these properties
- The expected format/constraints of serviceInstanceId
- Why "appsmith-0" was chosen as the default
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/public/index.html (1 hunks)
🔇 Additional comments (1)
app/client/public/index.html (1)
232-233: Verify the default values.Please confirm that:
- "self-hosted" is the appropriate default for deploymentName
- "appsmith-0" is the intended default for serviceInstanceId
✅ Verification successful
Default values are consistent across the codebase
The verification confirms that both default values are consistently used:
- "self-hosted" appears as the default deploymentName in multiple configuration files and utilities
- "appsmith-0" is used as the default serviceInstanceId in the same configuration contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of these default values to ensure consistency rg "self-hosted" --type ts --type js rg "appsmith-0" --type ts --type jsLength of output: 1220
|
Deploy-Preview-URL: https://ce-37101.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11522024377. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/src/sagas/ActionExecution/PluginActionSaga.ts (3)
Line range hint
1442-1452: Consider enhancing error type granularity.The error handling could be more specific by:
- Creating distinct error types for different failure scenarios
- Adding error codes for better error tracking
- const isError = isErrorResponse(response); + const { errorCode, isError } = isErrorResponse(response); yield validateResponse(response); payload = createActionExecutionResponse(response); + if (isError) { + payload.errorCode = errorCode; + }
Line range hint
1439-1452: Consider adding rate limiting for action execution.To prevent abuse, consider implementing rate limiting for action execution, especially for file uploads and resource-intensive operations.
Line range hint
1439-1452: Add JSDoc comments for better documentation.Consider adding JSDoc comments to document the response format and possible error scenarios.
+/** + * Executes the action and handles the response + * @param formData - The form data containing action parameters + * @param timeout - Optional timeout for the action + * @returns {ActionExecutionResponse} - The execution response + * @throws {PluginActionExecutionError} - When execution fails + */ response = yield ActionAPI.executeAction(formData, timeout);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/UITelemetry/generateTraces.ts (0 hunks)
- app/client/src/api/ActionAPI.tsx (1 hunks)
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts (1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/UITelemetry/generateTraces.ts
🧰 Additional context used
🪛 Biome
app/client/src/api/ActionAPI.tsx
[error] 237-237: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments (1)
app/client/src/sagas/ActionExecution/PluginActionSaga.ts (1)
1439-1439: Verify tracing functionality after parameter removal.The removal of
parentSpanparameter fromActionAPI.executeActioncall might affect distributed tracing capabilities.✅ Verification successful
Based on the search results, I can see that the tracing functionality is well-maintained throughout the codebase. Let me verify the specific changes to
ActionAPI.executeActionto ensure the tracing is handled correctly.
Let me check how tracing is handled in the ActionAPI implementation.
Let me check the executeAction method implementation specifically.
Let me try a broader search to find the executeAction method.
Removal of parentSpan parameter is safe and aligned with codebase changes
The removal of
parentSpanparameter fromActionAPI.executeActionis correct as:
- The method now only handles core API execution with
formDataandtimeout- Tracing is properly managed at the saga level where
executeActionis called- The codebase shows consistent tracing patterns using
startRootSpanandstartNestedSpanat appropriate abstraction levels🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other tracing-related code that might be affected rg -A 5 'parentSpan|OtlpSpan|startRootSpan|endSpan' --type tsLength of output: 39447
Script:
#!/bin/bash # Search for ActionAPI.executeAction implementation ast-grep --pattern 'executeAction($$$) { $$$ }' # Also check the file directly rg -A 10 "executeAction.*formData.*timeout" --type tsLength of output: 1044
Script:
#!/bin/bash # Search for ActionAPI implementation and its executeAction method rg -A 10 "class ActionAPI" --type tsLength of output: 796
Script:
#!/bin/bash # Search for executeAction method in ActionAPI rg -B 5 -A 15 "executeAction.*formData.*timeout" app/client/src/api/ActionAPI.tsxLength of output: 83
Script:
#!/bin/bash # Search for executeAction method with a broader pattern ast-grep --pattern 'executeAction($$$) { $$$ }' # Also try with ripgrep for any executeAction definition rg -B 5 -A 15 "static.*executeAction" --type tsLength of output: 1431
| return wrapFnWithParentTraceContext(parentSpan, async () => { | ||
| return await this.executeApiCall(executeAction, timeout); | ||
| }); | ||
| return await this.executeApiCall(executeAction, timeout); |
There was a problem hiding this comment.
Replace this with class name in static context.
Using this in a static method can be confusing and is considered a bad practice.
Apply this fix:
- return await this.executeApiCall(executeAction, timeout);
+ return await ActionAPI.executeApiCall(executeAction, timeout);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return await this.executeApiCall(executeAction, timeout); | |
| return await ActionAPI.executeApiCall(executeAction, timeout); |
🧰 Tools
🪛 Biome
[error] 237-237: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
|
Deploy-Preview-URL: https://ce-37101.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11565878135. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/UITelemetry/auto-otel-web.ts (4 hunks)
🔇 Additional comments (3)
app/client/src/UITelemetry/auto-otel-web.ts (3)
7-10: LGTM: Updated semantic convention imports
The migration to standardized OpenTelemetry semantic conventions improves maintainability.
29-32: LGTM: Clean configuration extraction
The destructuring pattern improves code readability and maintainability.
111-115: Previous review comment about XHR configuration remains valid
The suggestion to add URL ignore patterns and header propagation configuration is still relevant for proper distributed tracing implementation.
| [ATTR_DEPLOYMENT_NAME]: deploymentName, | ||
| [ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId, | ||
| [ATTR_SERVICE_NAME]: serviceName, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting duplicate resource configuration
The same resource configuration is duplicated between tracer and meter providers.
Consider extracting the resource configuration:
+const telemetryResource = new Resource({
+ [ATTR_DEPLOYMENT_NAME]: deploymentName,
+ [ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId,
+ [ATTR_SERVICE_NAME]: serviceName,
+});
const tracerProvider = new WebTracerProvider({
- resource: new Resource({
- [ATTR_DEPLOYMENT_NAME]: deploymentName,
- [ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId,
- [ATTR_SERVICE_NAME]: serviceName,
- }),
+ resource: telemetryResource,
});
const meterProvider = new MeterProvider({
- resource: new Resource({
- [ATTR_DEPLOYMENT_NAME]: deploymentName,
- [ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId,
- [ATTR_SERVICE_NAME]: serviceName,
- }),
+ resource: telemetryResource,Also applies to: 85-87
|
Deploy-Preview-URL: https://ce-37101.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/jest.config.js (1)
102-105: Consider extracting common configuration patterns.
The observability configuration follows the same pattern as other feature configurations. Consider creating a shared configuration schema to ensure consistency across different environments.
This would help:
- Maintain consistency across environments
- Validate configuration structure
- Make updates more maintainable
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/jest.config.js (1 hunks)
🔇 Additional comments (1)
app/client/jest.config.js (1)
102-105: LGTM! Verify consistency with other environments.
The observability configuration is appropriately set up for the Jest test environment.
Let's verify similar configuration exists in other environment files:
✅ Verification successful
Observability configuration is consistent with the application's setup
The observability configuration in jest.config.js aligns with the application-wide observability settings found in:
- app/client/src/ce/configs/types.ts (type definitions)
- app/client/src/ce/configs/index.ts (configuration implementation)
- app/client/src/UITelemetry/auto-otel-web.ts (telemetry usage)
The values match the expected format with deploymentName and serviceInstanceId properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for observability configuration patterns in other environment files
rg -g '*.{js,ts,json}' -A 2 'observability.*:.*{'
Length of output: 153
Script:
#!/bin/bash
# Search for observability configuration patterns in other environment files
rg -g '*.{js,ts,json}' "observability.*deploymentName|observability.*serviceInstanceId" -A 2
Length of output: 691
Script:
#!/bin/bash
# Search for any observability configuration patterns
rg -g '*.{js,ts,json}' "observability" -B 2 -A 2
Length of output: 183064
Script:
#!/bin/bash
# Look for configuration files that might contain observability settings
fd -e js -e json -e ts "config|settings" --type f --exec grep -l "observability" {} \;
Length of output: 117
Description
Use consistent properties for OTLP and propagate to server.
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
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11568095483
Commit: 0cefb16
Cypress dashboard.
Tags: @tag.Sanity
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Tue, 29 Oct 2024 06:03:03 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
executeActionmethod by removing unnecessary telemetry complexity.Documentation