Skip to content

chore: Add OTLP traces to RTS #36788

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 5 commits into from
Oct 10, 2024
Merged

chore: Add OTLP traces to RTS #36788

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d4c949c
chore: Add OTLP traces to RTS
nidhi-nair Oct 9, 2024
8087649
Add fallback for unconfigured deployment
nidhi-nair Oct 9, 2024
6c3e630
Lint fixes
nidhi-nair Oct 9, 2024
714d989
Use hostname to retrieve container identifier
nidhi-nair Oct 10, 2024
d4e3bfe
Remove debug logs for instrumentation
nidhi-nair Oct 10, 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
3 changes: 3 additions & 0 deletions app/client/packages/rts/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
"start": "./start-server.sh"
},
"dependencies": {
"@opentelemetry/instrumentation-http": "^0.53.0",
"@opentelemetry/sdk-trace-node": "^1.26.0",
"@opentelemetry/semantic-conventions": "^1.27.0",
"@shared/ast": "workspace:^",
"axios": "^1.7.4",
"express": "^4.20.0",
Expand Down
52 changes: 52 additions & 0 deletions app/client/packages/rts/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import {
BatchSpanProcessor,
NodeTracerProvider
} from "@opentelemetry/sdk-trace-node";
import { Resource } from "@opentelemetry/resources";
import { ATTR_DEPLOYMENT_NAME, ATTR_SERVICE_INSTANCE_ID } from "@opentelemetry/semantic-conventions/incubating";

@coderabbitai coderabbitai Bot Oct 9, 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.

🛠️ Refactor suggestion

Consider the stability of incubating semantic conventions

Class, when we're importing attributes from @opentelemetry/semantic-conventions/incubating, we need to be cautious. These incubating features might not be stable and could change in future updates, which can lead to maintenance challenges. It's often better to use stable conventions unless there's a specific need for the incubating ones. Let's think about whether we can use stable alternatives or plan to monitor changes in these conventions.

import { ATTR_SERVICE_NAME } from "@opentelemetry/semantic-conventions";
import { diag, DiagConsoleLogger, DiagLogLevel } from "@opentelemetry/api";
import { registerInstrumentations } from "@opentelemetry/instrumentation";
import { HttpInstrumentation } from "@opentelemetry/instrumentation-http";
import { OTLPTraceExporter } from "@opentelemetry/exporter-trace-otlp-proto";

const provider = new NodeTracerProvider({
resource: new Resource({
[ATTR_DEPLOYMENT_NAME]: `${process.env.APPSMITH_DEPLOYMENT_NAME}`,

@coderabbitai coderabbitai Bot Oct 9, 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.

⚠️ Potential issue

Ensure 'APPSMITH_DEPLOYMENT_NAME' is properly set

Students, it's crucial to make sure that environment variables like APPSMITH_DEPLOYMENT_NAME are defined before we use them. If this variable isn't set, we might end up with an 'undefined' value, which can cause problems in our resource attributes. Consider adding a default value or validating that the variable is present to prevent unexpected behavior.

[ATTR_SERVICE_INSTANCE_ID]: `${process.env.NEW_RELIC_METADATA_KUBERNETES_POD_NAME || "appsmith-0"}`,

@dvj1988 dvj1988 Oct 10, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could we use the HOSTNAME variable instead? I believe its the pod name in Kubernetes and the container ID in Docker.

[ATTR_SERVICE_NAME]: "rts",
}),
});

const nrTracesExporter = new OTLPTraceExporter({
url: `${process.env.APPSMITH_NEW_RELIC_OTEL_EXPORTER_OTLP_ENDPOINT}/v1/traces`,
headers: {
"api-key": `${process.env.APPSMITH_NEW_RELIC_OTLP_LICENSE_KEY}`,
Comment on lines +25 to +27

@coderabbitai coderabbitai Bot Oct 9, 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.

⚠️ Potential issue

Validate critical environment variables for exporter configuration

Let's not forget to verify that APPSMITH_NEW_RELIC_OTEL_EXPORTER_OTLP_ENDPOINT and APPSMITH_NEW_RELIC_OTLP_LICENSE_KEY are set properly. Without these, our OTLPTraceExporter won't function correctly, and we won't be able to export traces as intended. To avoid runtime errors, we should add checks or error handling to ensure these environment variables are available.

},
});

registerInstrumentations({
instrumentations: [new HttpInstrumentation()],
});

const batchSpanProcessor = new BatchSpanProcessor(
nrTracesExporter,
//Optional BatchSpanProcessor Configurations
{
// The maximum queue size. After the size is reached spans are dropped.
maxQueueSize: 100,
// The maximum batch size of every export. It must be smaller or equal to maxQueueSize.
maxExportBatchSize: 50,
// The interval between two consecutive exports
scheduledDelayMillis: 500,
// How long the export can run before it is cancelled
exportTimeoutMillis: 30000,
},
);

provider.addSpanProcessor(batchSpanProcessor);
provider.register();

diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.DEBUG);

@coderabbitai coderabbitai Bot Oct 9, 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.

🛠️ Refactor suggestion

Adjust diagnostic log level based on environment

Remember, setting DiagLogLevel.DEBUG in a production environment can lead to overly verbose logging and may impact performance. It's a good practice to set the log level according to the environment—use DEBUG during development and a less verbose level like INFO or WARN in production. We can achieve this by configuring the log level through an environment variable.

@dvj1988 dvj1988 Oct 10, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@nidhi-nair I missed this line. Can you remove this?


console.log(">>>>>> Tracer initialized");
1 change: 1 addition & 0 deletions app/client/packages/rts/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import './instrumentation';
import http from "http";
import express from "express";
import { Server } from "socket.io";
Expand Down
126 changes: 120 additions & 6 deletions app/client/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4769,13 +4769,31 @@ __metadata:
languageName: node
linkType: hard

"@opentelemetry/api-logs@npm:0.53.0":
version: 0.53.0
resolution: "@opentelemetry/api-logs@npm:0.53.0"
dependencies:
"@opentelemetry/api": ^1.0.0
checksum: 3383ff75f94a77402370a655f8edf049f9864ad60140f70821a1b775ce43bdb9ca6fade533a1faf46dbca19f3189bcbf1f8805062f5a68bfe2a00281b1712d1f
languageName: node
linkType: hard

"@opentelemetry/api@npm:^1.0.0, @opentelemetry/api@npm:^1.9.0":
version: 1.9.0
resolution: "@opentelemetry/api@npm:1.9.0"
checksum: 9e88e59d53ced668f3daaecfd721071c5b85a67dd386f1c6f051d1be54375d850016c881f656ffbe9a03bedae85f7e89c2f2b635313f9c9b195ad033cdc31020
languageName: node
linkType: hard

"@opentelemetry/context-async-hooks@npm:1.26.0":
version: 1.26.0
resolution: "@opentelemetry/context-async-hooks@npm:1.26.0"
peerDependencies:
"@opentelemetry/api": ">=1.0.0 <1.10.0"
checksum: f0fe5bfa3aeed99fbe7d6f6157e3bcc2e4450850a62ef60e551812f3e5aa72cb81e38de8c4e1b6934c93e18579a503664597f78e7e7d9904e271f59c939a3e02
languageName: node
linkType: hard

"@opentelemetry/context-zone-peer-dep@npm:1.25.1":
version: 1.25.1
resolution: "@opentelemetry/context-zone-peer-dep@npm:1.25.1"
Expand Down Expand Up @@ -4807,7 +4825,7 @@ __metadata:
languageName: node
linkType: hard

"@opentelemetry/core@npm:^1.26.0":
"@opentelemetry/core@npm:1.26.0, @opentelemetry/core@npm:^1.26.0":
version: 1.26.0
resolution: "@opentelemetry/core@npm:1.26.0"
dependencies:
Expand Down Expand Up @@ -4848,6 +4866,20 @@ __metadata:
languageName: node
linkType: hard

"@opentelemetry/instrumentation-http@npm:^0.53.0":
version: 0.53.0
resolution: "@opentelemetry/instrumentation-http@npm:0.53.0"
dependencies:
"@opentelemetry/core": 1.26.0
"@opentelemetry/instrumentation": 0.53.0
"@opentelemetry/semantic-conventions": 1.27.0
semver: ^7.5.2
peerDependencies:
"@opentelemetry/api": ^1.3.0
checksum: 4ee569f7fc8c7ce50fabaff016d33577f36e63272b0634ac45806d70bffdf38fcf09db3cd9dd27c3150f6c4547fec673c356c419a6ed2399ff2849b9487a6e89
languageName: node
linkType: hard

"@opentelemetry/instrumentation@npm:0.52.1":
version: 0.52.1
resolution: "@opentelemetry/instrumentation@npm:0.52.1"
Expand All @@ -4864,6 +4896,22 @@ __metadata:
languageName: node
linkType: hard

"@opentelemetry/instrumentation@npm:0.53.0":
version: 0.53.0
resolution: "@opentelemetry/instrumentation@npm:0.53.0"
dependencies:
"@opentelemetry/api-logs": 0.53.0
"@types/shimmer": ^1.2.0
import-in-the-middle: ^1.8.1
require-in-the-middle: ^7.1.1
semver: ^7.5.2
shimmer: ^1.2.1
peerDependencies:
"@opentelemetry/api": ^1.3.0
checksum: a386fe066eab71129a6edbc883ab407b1022850e8acc4750029a12e8730588a8b81442d0b008aaddb46f7614af40d19d331e7348790ca2d08ba8eed6d23ffdae
languageName: node
linkType: hard

"@opentelemetry/otlp-exporter-base@npm:0.52.1":
version: 0.52.1
resolution: "@opentelemetry/otlp-exporter-base@npm:0.52.1"
Expand Down Expand Up @@ -4893,6 +4941,28 @@ __metadata:
languageName: node
linkType: hard

"@opentelemetry/propagator-b3@npm:1.26.0":
version: 1.26.0
resolution: "@opentelemetry/propagator-b3@npm:1.26.0"
dependencies:
"@opentelemetry/core": 1.26.0
peerDependencies:
"@opentelemetry/api": ">=1.0.0 <1.10.0"
checksum: c2e99a8ed2814cf5b8e6e2a79411f2f6d668b7d5fc8351e5302ea4149601a96ec655422cf59470c66d8a408850f8a6b5156bf7deac7afb07d3f7a935c51fff04
languageName: node
linkType: hard

"@opentelemetry/propagator-jaeger@npm:1.26.0":
version: 1.26.0
resolution: "@opentelemetry/propagator-jaeger@npm:1.26.0"
dependencies:
"@opentelemetry/core": 1.26.0
peerDependencies:
"@opentelemetry/api": ">=1.0.0 <1.10.0"
checksum: a0ac3888c86f1b4671c7ca520396b89b4c47fa9e9d976bd014472d2b7786e7c5bdf4823a6e2a900fed5ea5dfe23eda0bdf6740e77c1352f2c0f82b13a71c03df
languageName: node
linkType: hard

"@opentelemetry/resources@npm:1.25.1":
version: 1.25.1
resolution: "@opentelemetry/resources@npm:1.25.1"
Expand All @@ -4905,6 +4975,18 @@ __metadata:
languageName: node
linkType: hard

"@opentelemetry/resources@npm:1.26.0":
version: 1.26.0
resolution: "@opentelemetry/resources@npm:1.26.0"
dependencies:
"@opentelemetry/core": 1.26.0
"@opentelemetry/semantic-conventions": 1.27.0
peerDependencies:
"@opentelemetry/api": ">=1.0.0 <1.10.0"
checksum: f70b0fdf4fb00c950bc30084818c92a5339f1be5d709bd681ab14453e877d6bb9f700324b8e65a0eabfeea618d01ed071abf9088e00fa0bf7f3305b1abad22cb
languageName: node
linkType: hard

"@opentelemetry/sdk-logs@npm:0.52.1":
version: 0.52.1
resolution: "@opentelemetry/sdk-logs@npm:0.52.1"
Expand Down Expand Up @@ -4944,6 +5026,35 @@ __metadata:
languageName: node
linkType: hard

"@opentelemetry/sdk-trace-base@npm:1.26.0":
version: 1.26.0
resolution: "@opentelemetry/sdk-trace-base@npm:1.26.0"
dependencies:
"@opentelemetry/core": 1.26.0
"@opentelemetry/resources": 1.26.0
"@opentelemetry/semantic-conventions": 1.27.0
peerDependencies:
"@opentelemetry/api": ">=1.0.0 <1.10.0"
checksum: a4f4ddf644fd0d79b2bd49e4377143688d2aa657643a470d8bed6696f26817598fb4e9f16ba2d8c237292af56f06eec56594a7b4cc417d4ea7e490a45a22113b
languageName: node
linkType: hard

"@opentelemetry/sdk-trace-node@npm:^1.26.0":
version: 1.26.0
resolution: "@opentelemetry/sdk-trace-node@npm:1.26.0"
dependencies:
"@opentelemetry/context-async-hooks": 1.26.0
"@opentelemetry/core": 1.26.0
"@opentelemetry/propagator-b3": 1.26.0
"@opentelemetry/propagator-jaeger": 1.26.0
"@opentelemetry/sdk-trace-base": 1.26.0
semver: ^7.5.2
peerDependencies:
"@opentelemetry/api": ">=1.0.0 <1.10.0"
checksum: 1d63bed8fc36496698919ccd25be3b7b0e0d0bf9478f413a26bdbfe0bf0d4166bf58bbbee2415fb2fe42d3008b5c32ec7e4e42f2cb6d18b665b349eb025c15eb
languageName: node
linkType: hard

"@opentelemetry/sdk-trace-web@npm:1.25.1":
version: 1.25.1
resolution: "@opentelemetry/sdk-trace-web@npm:1.25.1"
Expand All @@ -4964,7 +5075,7 @@ __metadata:
languageName: node
linkType: hard

"@opentelemetry/semantic-conventions@npm:1.27.0":
"@opentelemetry/semantic-conventions@npm:1.27.0, @opentelemetry/semantic-conventions@npm:^1.27.0":

@nidhi-nair nidhi-nair Oct 10, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dvj1988 can you help me understand what this line implies? I was confused about having to install semantic conventions for using those constants when it was already present in repo

@dvj1988 dvj1988 Oct 10, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This package is already a dependency of the package @opentelemetry/core@npm:^1.26.0 which was added to app/client/package.json. When you added a caret to the version in the rts package.json the yarn workspace just added this to resolve it to the already existing package. You can just leave it as is.

version: 1.27.0
resolution: "@opentelemetry/semantic-conventions@npm:1.27.0"
checksum: 26d85f8d13c8c64024f7a84528cff41d56afc9829e7ff8a654576404f8b2c1a9c264adcc6fa5a9551bacdd938a4a464041fa9493e0a722e5605f2c2ae6752398
Expand Down Expand Up @@ -11023,10 +11134,10 @@ __metadata:
languageName: node
linkType: hard

"@types/shimmer@npm:^1.0.2":
version: 1.0.4
resolution: "@types/shimmer@npm:1.0.4"
checksum: f1e7f8b773c34ea21b69686cb100117bd94cc0d1f043e3fc50683453b9936d1295c4f48e1872766556234a9ec48ea37fc7e6b5e56212f66ec65d5b2b5d73092b
"@types/shimmer@npm:^1.0.2, @types/shimmer@npm:^1.2.0":
version: 1.2.0
resolution: "@types/shimmer@npm:1.2.0"
checksum: f081a31d826ce7bfe8cc7ba8129d2b1dffae44fd580eba4fcf741237646c4c2494ae6de2cada4b7713d138f35f4bc512dbf01311d813dee82020f97d7d8c491c
languageName: node
linkType: hard

Expand Down Expand Up @@ -12570,6 +12681,9 @@ __metadata:
version: 0.0.0-use.local
resolution: "appsmith-rts@workspace:packages/rts"
dependencies:
"@opentelemetry/instrumentation-http": ^0.53.0
"@opentelemetry/sdk-trace-node": ^1.26.0
"@opentelemetry/semantic-conventions": ^1.27.0
"@shared/ast": "workspace:^"
"@types/express": ^4.17.14
"@types/jest": ^29.2.3
Expand Down