chore: Removed manipulation for custom traceparent header#37121
chore: Removed manipulation for custom traceparent header#37121nidhi-nair merged 6 commits intoreleasefrom
Conversation
WalkthroughThe changes in this pull request involve the removal of 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
|
|
/build-deploy-preview skip-tests=true |
github-actions
bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11568406165. |
|
Deploy-Preview-URL: https://ce-37121.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11568951892. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| [ATTR_DEPLOYMENT_NAME]: deploymentName, | ||
| [ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId, | ||
| [ATTR_SERVICE_NAME]: serviceName, | ||
| "extra": "extra", |
There was a problem hiding this comment.
💡 Codebase verification
Remove the arbitrary "extra": "extra" resource attribute
The "extra": "extra" attribute:
- Is not used anywhere else in the codebase
- Doesn't follow the standard resource attributes pattern seen in other OpenTelemetry configurations
- Adds unnecessary noise to telemetry data
- Is inconsistent with other Resource configurations in the codebase that only use standard ATTR_ constants
🔗 Analysis chain
Remove arbitrary resource attribute.
The addition of "extra": "extra" seems unnecessary and doesn't follow OpenTelemetry semantic conventions. This attribute will be attached to all telemetry data without providing meaningful context.
- "extra": "extra",Let's check if this attribute is referenced elsewhere:
Let me verify if this attribute is part of any configuration or has any dependencies.
Let me check one more aspect to see if this is related to any OpenTelemetry configuration or testing setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to this extra attribute
rg -i '"extra":\s*"extra"'
Length of output: 94
Script:
#!/bin/bash
# Search for any configuration files or tests that might reference this attribute
rg -l -g '*.{ts,js,json}' -i 'extra' app/client/src/UITelemetry/
# Check if this is part of any interface or type definition
ast-grep --pattern 'interface $_ { $$$ extra: $_; $$$ }'
# Look for any OpenTelemetry configuration patterns
rg -A 5 -B 5 'Resource.default\(\)' app/client/src/UITelemetry/
Length of output: 233
Script:
#!/bin/bash
# Look for the context around this attribute in the file
rg -A 10 -B 10 '"extra":\s*"extra"' app/client/src/UITelemetry/auto-otel-web.ts
# Check for any test files that might be using this
fd -e test.ts -e spec.ts . app/client/src/UITelemetry/ --exec rg -l -i 'extra'
# Look for any resource attribute configurations
ast-grep --pattern 'new Resource({ $$$ })'
Length of output: 2208
|
Deploy-Preview-URL: https://ce-37121.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11570330015. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/MDCConstants.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (2 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (2)
20-21: LGTM: Clean import additions for tracing constants
76-79: Verify trace header format compliance
The implementation looks clean, but we should ensure the TRACE_ID value follows the W3C trace context format before setting it as TRACE_PARENT.
✅ Verification successful
Let me gather more context about the trace ID generation and validation.
Let me check one more aspect about the trace context generation.
Trace header implementation is properly validated
The TRACE_ID validation is handled by the Micrometer tracing framework through TraceContext builder in ObservationHelperImpl. The framework ensures W3C trace context compliance, making additional validation in RTSCaller unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TRACE_ID format validation or generation in the codebase
rg -g '!*.{log,lock}' -A 3 "TRACE_ID.*format|TRACE_ID.*generate|TraceContext"
Length of output: 1208
Script:
#!/bin/bash
# Look for TRACE_ID usage and generation in ObservationHelperImpl
ast-grep --pattern 'class ObservationHelperImpl {
$$$
$_($$) {
$$$
TRACE_ID
$$$
}
$$$
}'
# Check for any trace ID format constants or validation
rg -g '*.java' -B 2 -A 2 'TRACE_ID.*=|TRACE_ID.*pattern|W3C.*trace'
Length of output: 941
Script:
#!/bin/bash
# Look for the traceContextBuilder implementation and usage
rg -g '*.java' -B 3 -A 3 'traceContextBuilder|TraceContext.*build'
# Check if there's any validation in RTSCaller
ast-grep --pattern 'class RTSCaller {
$$$
makeRequest($$$) {
$$$
}
}'
Length of output: 1183
| public static final String THREAD = "thread"; | ||
| public static final String OBSERVATION = "micrometer.observation"; | ||
| public static final String TRACE_ID = "traceId"; | ||
| public static final String TRACE_PARENT = "traceparent"; |
There was a problem hiding this comment.
💡 Codebase verification
Potential issue with trace context propagation in RTSCaller
The TRACE_PARENT constant is being used incorrectly in RTSCaller.java. It sets the traceparent header value to just the trace ID, but according to W3C Trace Context spec, the traceparent header should follow the format: version-trace_id-parent_id-trace_flags.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java: The header value should be properly formatted instead of just usingcontextMap.get(TRACE_ID)
🔗 Analysis chain
LGTM! The constant follows W3C trace context specification.
The addition of TRACE_PARENT constant aligns with the standardization of trace propagation headers.
Let's verify the usage of this constant across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of TRACE_PARENT constant and any remaining custom traceparent header manipulations
# Test 1: Find all references to the new constant
echo "=== References to TRACE_PARENT constant ==="
rg "TRACE_PARENT" -A 2
# Test 2: Check for any remaining custom traceparent header manipulations
echo -e "\n=== Check for custom traceparent manipulations ==="
rg -i "traceparent.*header" -g '!*.{md,txt}'
Length of output: 1508
Script:
#!/bin/bash
# Let's verify if there are any other occurrences of traceparent being used directly as a string
# or any other header manipulations related to trace context
# Check for string literal "traceparent" usage
echo "=== Direct traceparent string usage ==="
rg -i '"traceparent"' --type java --type typescript --type javascript -A 2
# Check for trace context related header manipulations
echo -e "\n=== Trace context header manipulations ==="
rg -i '(trace|span).*header' --type java --type typescript --type javascript -A 2
# Check RTSCaller.java implementation to understand the trace context usage
echo -e "\n=== RTSCaller.java implementation ==="
cat app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java
Length of output: 4799
|
Deploy-Preview-URL: https://ce-37121.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11572125686. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (1)
37-39: Consider adding null check for observationRegistry.
While Spring typically ensures non-null dependencies, adding a null check would improve robustness.
public RTSCaller(ObservationRegistry observationRegistry) {
+ if (observationRegistry == null) {
+ throw new IllegalArgumentException("observationRegistry cannot be null");
+ }
this.observationRegistry = observationRegistry;
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (4 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (3)
3-3: LGTM: Clean addition of observation registry.
The import and field declaration follow Java best practices.
Also applies to: 28-28
63-63: LGTM: Proper integration of observation registry.
The WebClient builder is correctly configured with the observation registry.
Line range hint 1-108: Verify complete removal of custom traceparent handling.
Let's ensure no other custom traceparent header manipulations exist in the codebase.
✅ Verification successful
Let me check if this constant is still being used in the codebase.
Custom traceparent handling has been properly removed
The only remaining references to traceparent are:
- A commented-out block in RTSCaller.java
- An unused constant definition in MDCConstants.java
Since the code is already commented out and the constant is not being used anywhere else in the codebase, the removal of custom traceparent handling is complete. The new ObservationRegistry integration will handle the tracing properly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining traceparent header manipulations
rg -i "traceparent" --type java
Length of output: 187
Script:
#!/bin/bash
# Search for usages of TRACE_PARENT constant
rg -A 3 "TRACE_PARENT" --type java
Length of output: 1127
| // if (contextMap.containsKey(TRACE_PARENT)) { | ||
| // spec.header(TRACE_PARENT, contextMap.get(TRACE_ID)); | ||
| // } |
There was a problem hiding this comment.
Remove commented out code.
Since the PR's objective is to remove custom traceparent header manipulation, this commented code should be deleted rather than left in place.
- // if (contextMap.containsKey(TRACE_PARENT)) {
- // spec.header(TRACE_PARENT, contextMap.get(TRACE_ID));
- // }📝 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.
| // if (contextMap.containsKey(TRACE_PARENT)) { | |
| // spec.header(TRACE_PARENT, contextMap.get(TRACE_ID)); | |
| // } |
|
Deploy-Preview-URL: https://ce-37121.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11572732420. |
|
Deploy-Preview-URL: https://ce-37121.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (1)
44-54: Update class documentation to reflect new behavior.
The class comment still mentions "filter just Appsmith specific spans" which is no longer accurate.
-/**
- * This configuration file creates beans that are required to filter just Appsmith specific spans
- */
+/**
+ * This configuration file creates beans for managing trace span export and server observations.
+ */📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (1 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (1)
44-54: Verify impact on trace volume and downstream systems.
The removal of span filtering will increase the volume of trace data being exported. This could affect performance and costs of your observability backend.
Consider implementing sampling if trace volume becomes a concern.
✅ Verification successful
Let me gather more information about the tracing configuration to better understand the impact.
Let me check the OpenTelemetry configuration to understand the sampling setup.
Span filtering removal is safe to proceed
The change to return all spans is aligned with standard OpenTelemetry practices. The system uses OpenTelemetry SDK with configurable sampling through environment variables, so trace volume can be controlled at deployment time without code changes. Previous custom filtering was likely redundant with modern observability platforms' capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any span volume configurations or sampling rates in the codebase
rg -i "sampling.*rate|span.*filter" --type java
Length of output: 449
Script:
#!/bin/bash
# Check for tracing configuration and sampling related code
rg -i "tracingconfig|samplerconfig|opentelemetry.*sampler" --type java
# Look for any other span filtering or processing logic
ast-grep --pattern 'class $_ implements SpanProcessor { $$$ }'
# Check for any trace exporter configurations
rg -i "trace.*export|opentelemetry.*export" --type java
Length of output: 1627
Script:
#!/bin/bash
# Check OpenTelemetry configuration and initialization
rg -i "opentelemetry.*sdk|tracerprovider|sdktraceprovider" --type java -A 5
# Look for any application properties related to tracing
fd -e properties -e yaml | xargs rg -i "trace|opentelemetry|otel"
Length of output: 3542
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11586709897. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1)
67-73: Consider documentation updates for observability changes.
Since this is part of a larger change to streamline tracing and observability:
- Update configuration documentation to reflect the new property structure
- Consider adding migration notes if other services depend on these properties
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/MetricsConfig.java (1)
72-77: LGTM! OpenTelemetry resource attributes properly configured.
The resource attributes are correctly set following OpenTelemetry SDK best practices. Consider adding documentation about these observability changes in your monitoring/observability guide.
Consider adding a code comment explaining the significance of these resource attributes for observability.
app/server/appsmith-server/src/main/resources/application.properties (1)
100-101: Remove temporary configuration comment.
The comment suggests this is a temporary configuration. Consider either removing the comment and property or documenting when this will be deprecated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- app/client/packages/rts/src/instrumentation.ts (1 hunks)
- app/client/src/ce/configs/index.ts (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/ConditionalOnMicrometerMetricsEnabled.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/MetricsConfig.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (0 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (3 hunks)
- app/server/appsmith-server/src/main/resources/application.properties (2 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java
🔇 Additional comments (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/ConditionalOnMicrometerMetricsEnabled.java (1)
18-18: LGTM! Verify property configuration.
The property path update aligns with the broader observability configuration changes.
Let's verify the property exists in application properties:
✅ Verification successful
Property configuration verified and correctly mapped
The new property path appsmith.observability.metrics.enabled is properly defined in application.properties and mapped to the environment variable APPSMITH_MICROMETER_METRICS_ENABLED with a default value of false.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new property exists in application properties files
# Expect: Property should be defined in at least one properties file
rg "appsmith\.observability\.metrics\.enabled" -g "*.properties"
Length of output: 219
app/client/packages/rts/src/instrumentation.ts (2)
19-19: LGTM: Service name change aligns with naming convention.
The change from "rts" to "appsmith-rts" provides better service identification in distributed traces.
19-19: Verify trace aggregation in New Relic.
Ensure that existing trace queries and dashboards are updated to use the new service name "appsmith-rts".
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1)
67-73: LGTM! Verify configuration updates.
The property namespace changes from micrometer to observability align well with the PR objectives. The default values are preserved, maintaining backward compatibility.
Let's verify the configuration updates:
✅ Verification successful
Property namespace changes are properly reflected in application.properties
The verification confirms that:
- All new
observabilitynamespace properties are correctly defined inapplication.properties - No legacy
micrometernamespace properties remain in the properties files - Environment variable mappings are preserved
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new property names are defined in application properties
# and if there are any remaining old property references
echo "Checking for new property definitions..."
rg "appsmith\.observability\.(metrics|tracing)" -g "*.properties"
echo "Checking for any remaining old property references..."
rg "appsmith\.micrometer\.(metrics|tracing)" -g "*.properties"
Length of output: 961
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/MetricsConfig.java (2)
36-39: LGTM! Constants follow OpenTelemetry semantic conventions.
The new constants align with OpenTelemetry's semantic conventions for service and deployment identification.
46-50: Verify application.properties configuration.
The new properties follow Spring's best practices. Please ensure the corresponding properties are defined in application.properties:
- appsmith.observability.deployment.name
- appsmith.observability.service.instance.id
✅ Verification successful
Properties are correctly defined in application.properties
Both properties are properly configured with default values:
appsmith.observability.deployment.namedefaults to "self-hosted"appsmith.observability.service.instance.iddefaults to "appsmith-0"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify property definitions in application.properties
rg "appsmith\.observability\.(deployment\.name|service\.instance\.id)" -g "application*.properties"
Length of output: 384
app/server/appsmith-server/src/main/resources/application.properties (1)
102-107: LGTM! Well-structured observability configuration.
The new observability configuration follows good practices with:
- Consistent naming convention
- Appropriate default values
- Environment variable overrides
app/client/src/ce/configs/index.ts (1)
286-286: Verify service name usage across the codebase.
Let's verify how this service name is used in trace propagation across the codebase.
✅ Verification successful
Service name is correctly used in OpenTelemetry configuration
The service name "appsmith-client" is properly used in the client-side OpenTelemetry configuration and is consistent with the tracing setup. The service name is used as a resource attribute following OpenTelemetry semantic conventions (ATTR_SERVICE_NAME).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to the service name in trace/observability related code
rg -l "appsmith-client" | grep -i "trace\|observ\|telemetry"
# Search for other potential service name definitions
rg -l "serviceName.*appsmith.*client"
Length of output: 135
Script:
#!/bin/bash
# Let's search for any trace/telemetry configuration or initialization code
rg -A 5 "initializeSentryTracing|initializeTracing|initializeTelemetry"
# Search for any trace/span related code that might use service names
rg -A 3 "createSpan|startSpan|tracer\.start"
# Look for any OpenTelemetry related imports and configurations
rg -A 2 "@opentelemetry|opentelemetry"
Length of output: 48755
| deploymentName: observabilityDeploymentName.value, | ||
| serviceInstanceId: observabilityServiceInstanceId.value, | ||
| serviceName: "frontend", | ||
| serviceName: "appsmith-client", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the service name configurable.
The hardcoded service name "appsmith-client" is added to observability configuration. While this aligns with standardizing trace propagation, consider making it configurable via environment variables for flexibility in different deployment scenarios.
Apply this diff to make the service name configurable:
- serviceName: "appsmith-client",
+ serviceName: ENV_CONFIG.observability?.serviceName || APPSMITH_FEATURE_CONFIGS?.observability?.serviceName || "appsmith-client",Add the corresponding type definition to the INJECTED_CONFIGS interface:
observability: {
deploymentName: string;
serviceInstanceId: string;
serviceName: string;
}
|
Deploy-Preview-URL: https://ce-37121.dp.appsmith.com |
| } | ||
|
|
||
| @Bean | ||
| SpanExportingPredicate onlyAppsmithSpans() { |
There was a problem hiding this comment.
@nidhi-nair now this has been removed. we will allow all spans? won't there be too many and cause clutter?
There was a problem hiding this comment.
I'd like to see if this is still true post spring upgrade since the DP is not showing as many spans anymore. We can revisit this if we see a spike perhaps? This allows us to fill the gaps in the trace lineage.
2 participants
Description
All Appsmith systems should always rely on a single traceparent header to propagate traces, removing incorrect manipulation to bring back consistency.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 3aeba4a yet
Wed, 30 Oct 2024 03:57:35 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Configuration Changes
These updates enhance the application's observability, allowing for better tracking and monitoring of performance metrics.