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 all 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
2 changes: 1 addition & 1 deletion app/client/packages/rts/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const provider = new NodeTracerProvider({
resource: new Resource({
[ATTR_DEPLOYMENT_NAME]: `${process.env.APPSMITH_DEPLOYMENT_NAME || "self-hosted"}`,
[ATTR_SERVICE_INSTANCE_ID]: `${process.env.HOSTNAME || "appsmith-0"}`,
[ATTR_SERVICE_NAME]: "rts",
[ATTR_SERVICE_NAME]: "appsmith-rts",
}),
});

Expand Down
2 changes: 1 addition & 1 deletion app/client/src/ce/configs/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ export const getAppsmithConfigs = (): AppsmithUIConfigs => {
observability: {
deploymentName: observabilityDeploymentName.value,
serviceInstanceId: observabilityServiceInstanceId.value,
serviceName: "frontend",
serviceName: "appsmith-client",
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

🛠️ 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;
}

},
fusioncharts: {
enabled: fusioncharts.enabled,
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";
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2024

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";
}
2 changes: 1 addition & 1 deletion .../src/main/java/com/appsmith/server/annotations/ConditionalOnMicrometerMetricsEnabled.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@
@Target({ElementType.TYPE, ElementType.METHOD})
@Retention(RetentionPolicy.RUNTIME)
@Documented
@ConditionalOnExpression("${appsmith.micrometer.metrics.enabled}")
@ConditionalOnExpression("${appsmith.observability.metrics.enabled}")
public @interface ConditionalOnMicrometerMetricsEnabled {}
6 changes: 3 additions & 3 deletions ...server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,13 @@ public class CommonConfig {
@Value("${disable.telemetry:true}")
private boolean isTelemetryDisabled;

@Value("${appsmith.micrometer.tracing.detail.enabled:false}")
@Value("${appsmith.observability.tracing.detail.enabled:false}")
private boolean tracingDetail;

@Value("${appsmith.micrometer.metrics.detail.enabled:false}")
@Value("${appsmith.observability.metrics.detail.enabled:false}")
private boolean metricsDetail;

@Value("${appsmith.micrometer.metrics.interval.millis:60000}")
@Value("${appsmith.observability.metrics.interval.millis:60000}")
private int metricsIntervalMillis;

private List<String> allowedDomains;
Expand Down
20 changes: 11 additions & 9 deletions ...erver/appsmith-server/src/main/java/com/appsmith/server/configurations/MetricsConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,21 @@ public class MetricsConfig {

public static final String NEW_RELIC_MICROMETER_METRICS_ENDPOINT = "https://otlp.nr-data.net:4317";
public static final String API_KEY = "api-key";
public static final String CONTAINER_NAME_KEY = "container.name";

public static final String DEPLOYMENT_NAME_KEY = "deployment.name";
public static final String SERVICE_INSTANCE_ID_KEY = "service.instance.id";
public static final String SERVICE_NAME_KEY = "service.name";
public static final String SERVICE_NAME = "appsmith-server";
public static final String INSTRUMENTATION_PROVIDER_KEY = "instrumentation.provider";
public static final String MICROMETER = "micrometer";

@Value("${appsmith.newrelic.micrometer.metrics.application.name}")
private String newRelicApplicationName;

@Value("${appsmith.newrelic.licensekey}")
private String newRelicKey;

@Value("${appsmith.newrelic.micrometer.metrics.container.name}")
private String newRelicContainerName;
@Value("${appsmith.observability.deployment.name}")
private String deploymentName;

@Value("${appsmith.observability.service.instance.id}")
private String serviceInstanceId;

private final CommonConfig commonConfig;

Expand All @@ -68,11 +69,12 @@ public OpenTelemetry openTelemetry() {
return OpenTelemetrySdk.builder()
.setMeterProvider(SdkMeterProvider.builder()
.setResource(Resource.getDefault().toBuilder()
.put(SERVICE_NAME_KEY, newRelicApplicationName)
.put(DEPLOYMENT_NAME_KEY, deploymentName)
.put(SERVICE_NAME_KEY, SERVICE_NAME)
// Include instrumentation.provider=micrometer to enable micrometer metrics
// experience in New Relic
.put(INSTRUMENTATION_PROVIDER_KEY, MICROMETER)
.put(CONTAINER_NAME_KEY, newRelicContainerName)
.put(SERVICE_INSTANCE_ID_KEY, serviceInstanceId)
.build())
.registerMetricReader(PeriodicMetricReader.builder(OtlpGrpcMetricExporter.builder()
.setEndpoint(NEW_RELIC_MICROMETER_METRICS_ENDPOINT)
Expand Down
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.

21 changes: 0 additions & 21 deletions ...erver/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,10 @@
import io.micrometer.observation.Observation;
import io.micrometer.observation.ObservationPredicate;
import io.micrometer.observation.ObservationView;
import io.micrometer.tracing.Span;
import io.micrometer.tracing.exporter.SpanExportingPredicate;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.server.reactive.observation.ServerRequestObservationContext;

import static com.appsmith.external.constants.spans.BaseSpan.APPSMITH_SPAN_PREFIX;
import static com.appsmith.external.constants.spans.BaseSpan.AUTHENTICATE;
import static com.appsmith.external.constants.spans.BaseSpan.AUTHORIZE;

/**
* This configuration file creates beans that are required to filter just Appsmith specific spans
*/
Expand Down Expand Up @@ -42,19 +36,4 @@ ObservationPredicate noActuatorServerObservations() {
}
};
}

@Bean
SpanExportingPredicate onlyAppsmithSpans() {
Copy link
Contributor

@ApekshaBhosale ApekshaBhosale Oct 30, 2024

Choose a reason for hiding this comment

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

@nidhi-nair now this has been removed. we will allow all spans? won't there be too many and cause clutter?

Copy link
Contributor Author

@nidhi-nair nidhi-nair Oct 30, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ApekshaBhosale ApekshaBhosale Oct 30, 2024

Choose a reason for hiding this comment

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

yes makes sense

return (finishedSpan) -> {
if ((finishedSpan.getKind() != null && finishedSpan.getKind().equals(Span.Kind.SERVER))
|| finishedSpan.getName().startsWith(APPSMITH_SPAN_PREFIX)
|| finishedSpan.getName().startsWith(AUTHENTICATE)
|| finishedSpan.getName().startsWith(AUTHORIZE)) {
// A span is either an http server request root or Appsmith specific or login related or signup related
return true;
} else {
return false;
}
};
}
}
8 changes: 8 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
@@ -1,5 +1,6 @@
package com.appsmith.server.helpers;

import io.micrometer.observation.ObservationRegistry;
import jakarta.annotation.PostConstruct;
import lombok.NonNull;
import org.springframework.beans.factory.annotation.Value;
Expand All @@ -24,13 +25,19 @@
@Component
public class RTSCaller {

private final ObservationRegistry observationRegistry;

private WebClient webClient;

@Value("${appsmith.rts.port:}")
private String rtsPort;

private static final int MAX_IN_MEMORY_SIZE_IN_BYTES = 16 * 1024 * 1024;

public RTSCaller(ObservationRegistry observationRegistry) {
this.observationRegistry = observationRegistry;
}

@PostConstruct
private void makeWebClient() {
if (isEmpty(rtsPort)) {
Expand All @@ -53,6 +60,7 @@ private void makeWebClient() {
.build())
.clientConnector(new ReactorClientHttpConnector(HttpClient.create(connectionProvider)))
.baseUrl("http://127.0.0.1:" + rtsPort)
.observationRegistry(observationRegistry)
.build();
}

Expand Down
21 changes: 11 additions & 10 deletions app/server/appsmith-server/src/main/resources/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
spring.application.name=appsmith-server
server.port=${PORT:8080}
# Allow the Spring context to close all active requests before shutting down the server
# Please ref: https://docs.spring.io/spring-boot/docs/2.3.0.RELEASE/reference/html/spring-boot-features.html#boot-features-graceful-shutdown
Expand Down Expand Up @@ -95,6 +96,16 @@ management.tracing.sampling.probability=${APPSMITH_SAMPLING_PROBABILITY:0.1}
management.prometheus.metrics.export.descriptions=true
management.metrics.distribution.percentiles-histogram.http.server.requests=true

# Observability and Micrometer related configs
# Keeping this license key around, until later
appsmith.newrelic.licensekey=${APPSMITH_NEW_RELIC_OTLP_LICENSE_KEY:}
appsmith.observability.deployment.name=${APPSMITH_DEPLOYMENT_NAME:self-hosted}
appsmith.observability.service.instance.id=${HOSTNAME:appsmith-0}
appsmith.observability.metrics.enabled=${APPSMITH_MICROMETER_METRICS_ENABLED:false}
appsmith.observability.tracing.detail.enabled=${APPSMITH_ENABLE_TRACING_DETAIL:false}
appsmith.observability.metrics.detail.enabled=${APPSMITH_ENABLE_METRICS_DETAIL:false}
appsmith.observability.metrics.interval.millis=${APPSMITH_METRICS_INTERVAL_MILLIS:60000}

# Support disabling signup with an environment variable
signup.disabled = ${APPSMITH_SIGNUP_DISABLED:false}
signup.allowed-domains=${APPSMITH_SIGNUP_ALLOWED_DOMAINS:}
Expand All @@ -117,15 +128,5 @@ appsmith.internal.password=${APPSMITH_INTERNAL_PASSWORD:}
# GIT stale index.lock file valid time
appsmith.index.lock.file.time=${APPSMITH_INDEX_LOCK_FILE_TIME:300}

# NewRelic and Micrometer related configs
appsmith.newrelic.licensekey=${APPSMITH_NEW_RELIC_OTLP_LICENSE_KEY:}
appsmith.newrelic.micrometer.metrics.container.name=${NEW_RELIC_METADATA_KUBERNETES_POD_NAME:appsmith-0}
appsmith.newrelic.micrometer.metrics.application.name=${APPSMITH_NEWRELIC_MICROMETER_SERVICE_NAME:}
spring.application.name=${OTEL_SERVICE_NAME:appsmith-anonymous}
appsmith.micrometer.metrics.enabled=${APPSMITH_MICROMETER_METRICS_ENABLED:false}
appsmith.micrometer.tracing.detail.enabled=${APPSMITH_ENABLE_TRACING_DETAIL:false}
appsmith.micrometer.metrics.detail.enabled=${APPSMITH_ENABLE_METRICS_DETAIL:false}
appsmith.micrometer.metrics.interval.millis=${APPSMITH_METRICS_INTERVAL_MILLIS:60000}

springdoc.api-docs.path=/v3/docs
springdoc.swagger-ui.path=/v3/swagger