Skip to content

chore: Distributed tracing for client #37101

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 29, 2024
Merged

chore: Distributed tracing for client #37101

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
554635b
chore: Distributed tracing for client
nidhi-nair Oct 25, 2024
732ab2d
chore: Distributed tracing for client
nidhi-nair Oct 25, 2024
576b5da
chore: Distributed tracing for client
nidhi-nair Oct 25, 2024
1cd8075
Remove w3c propagator since it is already the default
nidhi-nair Oct 29, 2024
c32f0d5
Remove w3c propagator since it is the default
nidhi-nair Oct 29, 2024
0cefb16
Defaults for jest
nidhi-nair Oct 29, 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
4 changes: 4 additions & 0 deletions app/client/jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ module.exports = {
"__APPSMITH_NEW_RELIC_OTEL_EXPORTER_OTLP_ENDPOINT__",
),
},
observability: {
deploymentName: "jest-run",
serviceInstanceId: "appsmith-0",
},
fusioncharts: {
licenseKey: parseConfig("__APPSMITH_FUSIONCHARTS_LICENSE_KEY__"),
},
Expand Down
3 changes: 2 additions & 1 deletion app/client/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
"@mantine/hooks": "^5.10.1",
"@newrelic/browser-agent": "^1.255.0",
"@opentelemetry/api": "^1.9.0",
"@opentelemetry/auto-instrumentations-web": "^0.41.0",
"@opentelemetry/context-zone": "1.25.1",
"@opentelemetry/core": "^1.26.0",
"@opentelemetry/exporter-metrics-otlp-http": "0.52.1",
Expand All @@ -81,7 +82,7 @@
"@opentelemetry/sdk-metrics": "1.25.1",
"@opentelemetry/sdk-trace-base": "1.25.1",
"@opentelemetry/sdk-trace-web": "1.25.1",
"@opentelemetry/semantic-conventions": "1.25.1",
"@opentelemetry/semantic-conventions": "^1.27.0",
"@react-spring/web": "^9.7.4",
"@react-types/shared": "^3.23.0",
"@redux-saga/core": "1.1.3",
Expand Down
4 changes: 4 additions & 0 deletions app/client/public/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,10 @@
apiKey: parseConfig('{{env "APPSMITH_SEGMENT_KEY"}}'),
ceKey: parseConfig('{{env "APPSMITH_SEGMENT_CE_KEY"}}'),
},
observability: {
deploymentName: parseConfig('{{env "APPSMITH_DEPLOYMENT_NAME"}}') || "self-hosted",
serviceInstanceId: parseConfig('{{env "HOSTNAME"}}') || "appsmith-0",
},
newRelic:{
enableNewRelic: parseConfig('{{env "APPSMITH_NEW_RELIC_ACCOUNT_ENABLE"}}'),
accountId: parseConfig('{{env "APPSMITH_NEW_RELIC_ACCOUNT_ID"}}'),
Expand Down
63 changes: 20 additions & 43 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 @@ -4,12 +4,11 @@ import { ZoneContextManager } from "@opentelemetry/context-zone";
import { OTLPTraceExporter } from "@opentelemetry/exporter-trace-otlp-proto";
import { Resource } from "@opentelemetry/resources";
import {
SEMRESATTRS_SERVICE_NAME,
SEMRESATTRS_SERVICE_VERSION,
SEMRESATTRS_SERVICE_INSTANCE_ID,
} from "@opentelemetry/semantic-conventions";
ATTR_DEPLOYMENT_NAME,
ATTR_SERVICE_INSTANCE_ID,
} from "@opentelemetry/semantic-conventions/incubating";
import { ATTR_SERVICE_NAME } from "@opentelemetry/semantic-conventions";
import { getAppsmithConfigs } from "ee/configs";
import { W3CTraceContextPropagator } from "@opentelemetry/core";
import {
MeterProvider,
PeriodicExportingMetricReader,
Expand All @@ -18,33 +17,29 @@ import {
OTLPMetricExporter,
AggregationTemporalityPreference,
} from "@opentelemetry/exporter-metrics-otlp-http";
import type { Context, TextMapSetter } from "@opentelemetry/api";
import { metrics } from "@opentelemetry/api";
import { registerInstrumentations } from "@opentelemetry/instrumentation";
import { PageLoadInstrumentation } from "./PageLoadInstrumentation";
import { getWebAutoInstrumentations } from "@opentelemetry/auto-instrumentations-web";

enum CompressionAlgorithm {
NONE = "none",
GZIP = "gzip",
}
const { newRelic } = getAppsmithConfigs();
const {
applicationId,
browserAgentEndpoint,
otlpEndpoint,
otlpLicenseKey,
otlpServiceName,
} = newRelic;
const { newRelic, observability } = getAppsmithConfigs();
const { browserAgentEndpoint, otlpEndpoint, otlpLicenseKey } = newRelic;

const { deploymentName, serviceInstanceId, serviceName } = observability;

// This base domain is used to filter out the Smartlook requests from the browser agent
// There are some requests made to subdomains of smartlook.cloud which will also be filtered out
const smartlookBaseDomain = "smartlook.cloud";

const tracerProvider = new WebTracerProvider({
resource: new Resource({
[SEMRESATTRS_SERVICE_NAME]: otlpServiceName,
[SEMRESATTRS_SERVICE_INSTANCE_ID]: applicationId,
[SEMRESATTRS_SERVICE_VERSION]: "1.0.0",
[ATTR_DEPLOYMENT_NAME]: deploymentName,
[ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId,
[ATTR_SERVICE_NAME]: serviceName,
Comment on lines +40 to +42
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.

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

}),
});

Expand All @@ -71,32 +66,9 @@ const processor = new BatchSpanProcessor(
},
);

const W3C_OTLP_TRACE_HEADER = "traceparent";
const CUSTOM_OTLP_TRACE_HEADER = "traceparent-otlp";

//We are overriding the default header "traceparent" used for trace context because the browser
// agent shares the same header's distributed tracing
class CustomW3CTraceContextPropagator extends W3CTraceContextPropagator {
inject(
context: Context,
carrier: Record<string, unknown>,
setter: TextMapSetter,
) {
// Call the original inject method to get the default traceparent header
super.inject(context, carrier, setter);

// Modify the carrier to use a different header
if (carrier[W3C_OTLP_TRACE_HEADER]) {
carrier[CUSTOM_OTLP_TRACE_HEADER] = carrier[W3C_OTLP_TRACE_HEADER];
delete carrier[W3C_OTLP_TRACE_HEADER]; // Remove the original traceparent header
}
}
}

tracerProvider.addSpanProcessor(processor);
tracerProvider.register({
contextManager: new ZoneContextManager(),
propagator: new CustomW3CTraceContextPropagator(),
});

const nrMetricsExporter = new OTLPMetricExporter({
Expand All @@ -110,9 +82,9 @@ const nrMetricsExporter = new OTLPMetricExporter({

const meterProvider = new MeterProvider({
resource: new Resource({
[SEMRESATTRS_SERVICE_NAME]: otlpServiceName,
[SEMRESATTRS_SERVICE_INSTANCE_ID]: applicationId,
[SEMRESATTRS_SERVICE_VERSION]: "1.0.0",
[ATTR_DEPLOYMENT_NAME]: deploymentName,
[ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId,
[ATTR_SERVICE_NAME]: serviceName,
}),
readers: [
new PeriodicExportingMetricReader({
Expand All @@ -136,5 +108,10 @@ registerInstrumentations({
smartlookBaseDomain,
],
}),
getWebAutoInstrumentations({
"@opentelemetry/instrumentation-xml-http-request": {
enabled: true,
},
}),
Comment on lines +111 to +115
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

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

🛠️ 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.

],
});
8 changes: 0 additions & 8 deletions app/client/src/UITelemetry/generateTraces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,6 @@ export const startAndEndSpanForFn = <T>(
return res;
};

// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function wrapFnWithParentTraceContext(parentSpan: Span, fn: () => any) {
const parentContext = trace.setSpan(context.active(), parentSpan);

return context.with(parentContext, fn);
}

export function startAndEndSpan(
spanName: string,
startTime: number,
Expand Down
11 changes: 1 addition & 10 deletions app/client/src/api/ActionAPI.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import axios from "axios";
import type { Action, ActionViewMode } from "entities/Action";
import type { APIRequest } from "constants/AppsmithActionConstants/ActionConstants";
import type { WidgetType } from "constants/WidgetConstants";
import type { OtlpSpan } from "UITelemetry/generateTraces";
import { wrapFnWithParentTraceContext } from "UITelemetry/generateTraces";
import type { ActionParentEntityTypeInterface } from "ee/entities/Engine/actionHelpers";

export interface Property {
Expand Down Expand Up @@ -233,17 +231,10 @@ class ActionAPI extends API {
static async executeAction(
executeAction: FormData,
timeout?: number,
parentSpan?: OtlpSpan,
): Promise<AxiosPromise<ActionExecutionResponse>> {
ActionAPI.abortActionExecutionTokenSource = axios.CancelToken.source();

if (!parentSpan) {
return this.executeApiCall(executeAction, timeout);
}

return wrapFnWithParentTraceContext(parentSpan, async () => {
return await this.executeApiCall(executeAction, timeout);
});
return await this.executeApiCall(executeAction, timeout);
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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)

}

static async moveAction(moveRequest: MoveActionRequest) {
Expand Down
30 changes: 21 additions & 9 deletions app/client/src/ce/configs/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,17 @@ export interface INJECTED_CONFIGS {
apiKey: string;
ceKey: string;
};
observability: {
deploymentName: string;
serviceInstanceId: string;
};
newRelic: {
enableNewRelic: boolean;
accountId: string;
applicationId: string;
browserAgentlicenseKey: string;
browserAgentEndpoint: string;
otlpLicenseKey: string;
otlpServiceName: string;
otlpEndpoint: string;
};
fusioncharts: {
Expand Down Expand Up @@ -92,6 +95,10 @@ export const getConfigsFromEnvVars = (): INJECTED_CONFIGS => {
indexName: process.env.REACT_APP_ALGOLIA_SEARCH_INDEX_NAME || "",
snippetIndex: process.env.REACT_APP_ALGOLIA_SNIPPET_INDEX_NAME || "",
},
observability: {
deploymentName: process.env.APPSMITH_DEPLOYMENT_NAME || "self-hosted",
serviceInstanceId: process.env.HOSTNAME || "appsmith-0",
},
newRelic: {
enableNewRelic: !!process.env.APPSMITH_NEW_RELIC_ACCOUNT_ENABLE,
accountId: process.env.APPSMITH_NEW_RELIC_ACCOUNT_ID || "",
Expand All @@ -102,8 +109,6 @@ export const getConfigsFromEnvVars = (): INJECTED_CONFIGS => {
process.env.APPSMITH_NEW_RELIC_BROWSER_AGENT_ENDPOINT || "",
otlpLicenseKey: process.env.APPSMITH_NEW_RELIC_OTLP_LICENSE_KEY || "",
otlpEndpoint: process.env.APPSMITH_NEW_RELIC_OTEL_SERVICE_NAME || "",
otlpServiceName:
process.env.APPSMITH_NEW_RELIC_OTEL_EXPORTER_OTLP_ENDPOINT || "",
},
logLevel:
(process.env.REACT_APP_CLIENT_LOG_LEVEL as
Expand Down Expand Up @@ -171,6 +176,14 @@ export const getAppsmithConfigs = (): AppsmithUIConfigs => {
ENV_CONFIG.mixpanel.apiKey,
APPSMITH_FEATURE_CONFIGS?.mixpanel.apiKey,
);
const observabilityDeploymentName = getConfig(
ENV_CONFIG.observability.deploymentName,
APPSMITH_FEATURE_CONFIGS?.observability.deploymentName,
);
const observabilityServiceInstanceId = getConfig(
ENV_CONFIG.observability.serviceInstanceId,
APPSMITH_FEATURE_CONFIGS?.observability.serviceInstanceId,
);
const newRelicAccountId = getConfig(
ENV_CONFIG.newRelic.accountId,
APPSMITH_FEATURE_CONFIGS?.newRelic.accountId,
Expand All @@ -191,11 +204,6 @@ export const getAppsmithConfigs = (): AppsmithUIConfigs => {
ENV_CONFIG.newRelic.otlpLicenseKey,
APPSMITH_FEATURE_CONFIGS?.newRelic.otlpLicenseKey,
);

const newRelicOtlpServiceName = getConfig(
ENV_CONFIG.newRelic.otlpServiceName,
APPSMITH_FEATURE_CONFIGS?.newRelic.otlpServiceName,
);
const newRelicOtlpEndpoint = getConfig(
ENV_CONFIG.newRelic.otlpEndpoint,
APPSMITH_FEATURE_CONFIGS?.newRelic.otlpEndpoint,
Expand Down Expand Up @@ -271,7 +279,11 @@ export const getAppsmithConfigs = (): AppsmithUIConfigs => {
browserAgentEndpoint: newRelicBrowserAgentEndpoint.value,
otlpLicenseKey: newRelicOtlpLicenseKey.value,
otlpEndpoint: newRelicOtlpEndpoint.value,
otlpServiceName: newRelicOtlpServiceName.value,
},
observability: {
deploymentName: observabilityDeploymentName.value,
serviceInstanceId: observabilityServiceInstanceId.value,
serviceName: "frontend",
},
fusioncharts: {
enabled: fusioncharts.enabled,
Expand Down
6 changes: 5 additions & 1 deletion app/client/src/ce/configs/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,18 @@ export interface AppsmithUIConfigs {
enabled: boolean;
id: string;
};
observability: {
deploymentName: string;
serviceInstanceId: string;
serviceName: string;
};
newRelic: {
enableNewRelic: boolean;
accountId: string;
applicationId: string;
browserAgentlicenseKey: string;
browserAgentEndpoint: string;
otlpLicenseKey: string;
otlpServiceName: string;
otlpEndpoint: string;
};
segment: {
Expand Down
2 changes: 1 addition & 1 deletion app/client/src/sagas/ActionExecution/PluginActionSaga.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,7 +1436,7 @@ function* executePluginActionSaga(
let response: ActionExecutionResponse;

try {
response = yield ActionAPI.executeAction(formData, timeout, parentSpan);
response = yield ActionAPI.executeAction(formData, timeout);

const isError = isErrorResponse(response);

Expand Down
Loading