Skip to content

chore: Removed manipulation for custom traceparent header #37121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nidhi-nair merged 6 commits into from
Oct 30, 2024
Merged

chore: Removed manipulation for custom traceparent header #37121

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a132f6a
chore: Removed manipulation for custom traceparent header
nidhi-nair Oct 29, 2024
0c7f7fb
Force client build
nidhi-nair Oct 29, 2024
62fb259
Added propagation of traceparent for rts
nidhi-nair Oct 29, 2024
4b4b6f2
Added propagation of traceparent for rts
nidhi-nair Oct 29, 2024
134dcf1
Added propagation of traceparent for rts
nidhi-nair Oct 29, 2024
3aeba4a
standardizing stuff on server
nidhi-nair Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/client/src/UITelemetry/auto-otel-web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const tracerProvider = new WebTracerProvider({
[ATTR_DEPLOYMENT_NAME]: deploymentName,
[ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId,
[ATTR_SERVICE_NAME]: serviceName,
"extra": "extra",

@coderabbitai coderabbitai Bot Oct 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

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

}),
});

Expand Down
1 change: 1 addition & 0 deletions ...erver/appsmith-interfaces/src/main/java/com/appsmith/external/constants/MDCConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ public class MDCConstants {
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";

@coderabbitai coderabbitai Bot Oct 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 using contextMap.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

public static final String SPAN_ID = "spanId";
}
78 changes: 0 additions & 78 deletions ...rver/src/main/java/com/appsmith/server/configurations/MicrometerTraceHeaderWebFilter.java
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import java.time.Duration;
import java.util.Map;

import static com.appsmith.external.constants.MDCConstants.TRACE_ID;
import static com.appsmith.external.constants.MDCConstants.TRACE_PARENT;
import static com.appsmith.server.filters.MDCFilter.INTERNAL_REQUEST_ID_HEADER;
import static com.appsmith.server.filters.MDCFilter.REQUEST_ID_HEADER;
import static org.apache.commons.lang3.StringUtils.isEmpty;
Expand Down Expand Up @@ -71,6 +73,10 @@ private Mono<WebClient.RequestBodySpec> makeRequest(HttpMethod method, String pa
spec.header(INTERNAL_REQUEST_ID_HEADER, contextMap.get(INTERNAL_REQUEST_ID_HEADER));
}

if (contextMap.containsKey(TRACE_ID)) {
spec.header(TRACE_PARENT, contextMap.get(TRACE_ID));
}

if (contextMap.containsKey(REQUEST_ID_HEADER)) {
spec.header(REQUEST_ID_HEADER, contextMap.get(REQUEST_ID_HEADER));
}
Expand Down