Update logging instrumentation #577

daniyelnnr · 2025-04-02T16:56:23Z

What is the purpose of this pull request?

Update logging instrumentation, adding telemetry and log clients and integrates node-vtex-api with the new version of the diagnostics library that supports OpenTelemetry protocols.
It also updates Typescript and Opentelemetry dependencies to enable support for node builder 6.x and IO apps using Node 16.x.

What problem is this solving?

This implementation of node-vtex-api is not using the newest version of diagnostics, which is VTEX's standard library for instrumentation and observability. This PR provides consistency in telemetry generation and allows it to adopt OTel protocol for logs.

How should this be manually tested?

To test it:

Clone this repository and checkout to update/logs-instrumentation branch;
Install the dependencies and generate the build:

  yarn && yarn build

Link the previously generated package:

  yarn link

Create an IO app (e.g. service-example)
Link this package to your app's node_modules

  cd ../<your-app>/node && yarn link @vtex/api

Add some logging to your app and monitor new logs being received by the collector through this dashboard in Grafana.

Types of changes

Bug fix (a non-breaking change which fixes an issue)
New feature (a non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Requires change to documentation, which has been updated accordingly.

daniyelnnr · 2025-04-02T17:03:19Z

src/service/logger/client.ts

+    const telemetryClient = await getTelemetryClient();
+
+    const logsConfig = Exporters.CreateLogsExporterConfig({
+      endpoint: process.env.OTEL_EXPORTER_OTLP_ENDPOINT || 'http://io-telemetry-http.vtex.systems:3333',


I couldn't identify the proper way to handle this address, at first I thought it should be in constants.ts but I'm not sure about that. Any ideas about it?

I don't know if we should have a default value for the exporter endpoint. Maybe if we can't initialize it with a environment variable, this instance just doesn't send logs anywhere (or logs them to stdout as a default). I'm afraid of a behaviour where for some reason, if the endpoints are unavailable, this client crashes the application.

The CreateLogsExporterConfig function receives an object with some content on it to generate the exporter config, but all these fields are optional as we can see in the function signature - and internally it can handle the scenario where an endpoint or any other field is unavailable.

So if we have problems with the environment variable and/or don't proper inform the collector endpoint, the worst case scenario we'll get an functional exporter that sends logs to an invalid collector due to misconfiguration.

But yet, I'm not sure how I should proper inform the endpoint of the collector here.

I believe I wouldn't have a hard-coded default endpoint, since—as you showed—the CreateLogsExporterConfig can handle the undefined case. That exposes internal information in a public lib and also creates a config that is hard to modify: we need to redeploy the runtime if want to change it.

In summary, I would only get rid of the endpoint and let it be process.env.OTEL_EXPORTER_OTPL_ENDPOINT

@arturpimentel I agree 100% about removing the hard-coded endpoint and will commit a change to fix this. However, I'm not sure where I could define the process.env.OTEL_EXPORTER_OTLP_ENDPOINT, as we don't want to set this in this lib. I imagine this info need to be defined somewhere else - is service-runtime-node the right place to do it?

arturpimentel · 2025-04-02T20:17:27Z

src/HttpClient/middlewares/cache.ts

+    let cached: void | Cached = undefined
    try {
      const cacheHasWithSegment = await storage.has(keyWithSegment)
      cached = cacheHasWithSegment ? await storage.get(keyWithSegment) : await storage.get(key)
-    } catch (error) {


These will probably conflict with the changes I made myself in #574. I believe I like your implementation better, so I might keep the changes you made here in the future.

arturpimentel · 2025-04-02T20:46:30Z

src/service/logger/client.ts

+    const telemetryClient = await getTelemetryClient();
+
+    const logsConfig = Exporters.CreateLogsExporterConfig({
+      endpoint: process.env.OTEL_EXPORTER_OTLP_ENDPOINT || 'http://io-telemetry-http.vtex.systems:3333',


I don't know if we should have a default value for the exporter endpoint. Maybe if we can't initialize it with a environment variable, this instance just doesn't send logs anywhere (or logs them to stdout as a default). I'm afraid of a behaviour where for some reason, if the endpoints are unavailable, this client crashes the application.

arturpimentel

Would you mind adding a high-level overview of the changes? I had some trouble understanding the logging architecture here. Maybe I'll use copilot for this, but I think it would be beneficial if I could picture how this is organized

silvadenisaraujo

Great to see this work materializing 🎉 added a few comments/questions.

src/service/logger/logger.ts

src/service/logger/client.ts

src/service/logger/logger.ts

src/service/telemetry/client.ts

src/service/logger/logger.ts

Copilot

Pull Request Overview

This pull request updates the logging instrumentation by integrating diagnostics and telemetry clients that support OpenTelemetry protocols. The changes include introducing asynchronous initialization for telemetry and log clients, updating various catch blocks with explicit type annotations, and adjusting cache behavior by replacing undefined with a null cast fallback.

Reviewed Changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/service/worker/runtime/events/middlewares/body.ts	Adjusted error catch type and import order for clarity.
src/service/worker/index.ts	Updated LogLevel import to use the new loggerTypes module.
src/service/tracing/tracingMiddlewares.ts	Added explicit catch type annotations.
src/service/telemetry/index.ts	Added an export for telemetry client functionality.
src/service/telemetry/client.ts	Introduced a TelemetryClientSingleton for managing telemetry client initialization.
src/service/logger/loggerTypes.ts	Added new logger type definitions.
src/service/logger/logger.ts	Refactored logger to use a new diagnostics-based log client with asynchronous initialization.
src/service/logger/index.ts	Exported additional modules including loggerTypes and client.
src/service/logger/console.ts	Adjusted LogLevel import to reference loggerTypes.
src/service/logger/client.ts	Implemented a new log client initialization using the diagnostics library.
src/service/index.ts, src/clients/infra/, src/HttpClient/middlewares/	Updated catch clauses with explicit type annotations.
src/caches/LRUDiskCache.ts, src/caches/DiskCache.ts	Replaced returning undefined with a null cast for fallback values.

Files not reviewed (1)

package.json: Language not supported

Comments suppressed due to low confidence (1)

src/caches/DiskCache.ts:46

Returning a casted null instead of undefined for a missing cache entry may cause inconsistencies if downstream code expects undefined. Consider reverting to undefined or ensuring type consistency across the cache API.

resolve(null as unknown as V)

src/service/logger/logger.ts

src/caches/LRUDiskCache.ts

daniyelnnr · 2025-04-08T02:24:19Z

Would you mind adding a high-level overview of the changes? I had some trouble understanding the logging architecture here. Maybe I'll use copilot for this, but I think it would be beneficial if I could picture how this is organized

@arturpimentel I followed your idea and used copilot to get an overview of the logging architecture - if you think it makes sense, I can add a summary of all the changes as well.

Original Logging Architecture

Proprietary logger with a specific VTEX format
Logs treated as separate entities from metrics and traces
Logs sent directly to stdout/stderr

[Application] → [VTEX Logger] → [stdout/stderr] → [Fluent bit] → [Log Collector]

Architecture after the changes

Logs follow the OpenTelemetry standard format
Logs automatically correlated with traces and spans
Logs exported via the OTLP standard
Centralization of exporter configuration
Context information and metadata propagated automatically.
Logs automatically enriched with trace IDs and span IDs.

[Application] → [Diagnostics Logger] → [OpenTelemetry SDK] → [OTLP Exporter] → [Telemetry Backend/Collector]

Here is a more detailed overview:

flowchart LR
    A[Application Code] --[Logger Instance]--> B["Diagnostics Logger /src/service/logger/client.ts"]
    M[Trace/Span Context] -.-> B
    Z["Diagnostics Metrics [TBD]"] -.-> X
    Y["Diagnostics Traces [TBD]"] -.-> X
    
    B --> X
    X[Telemetry Client]--> C["OpenTelemetry Logs (@opentelemetry/sdk-logs)"]
    D["Resource & Attributes (Context enrichment)"] --> C
    
    
    E --> F["o11y Collector
    (io-telemetry-http.vtex.systems)"]
    
    subgraph Unified Telemetry
        C --> E["OTLP Exporter (gRPC/HTTP)"]
        C --> P["stdout Exporter (gRPC/HTTP)"]
        G["OpenTelemetry Metrics (@opentelemetry/sdk-metrics)"] --> E
        G["OpenTelemetry Metrics (@opentelemetry/sdk-metrics)"] --> P
        H["OpenTelemetry Traces (@opentelemetry/sdk-trace)"] --> E
        H["OpenTelemetry Traces (@opentelemetry/sdk-trace)"] --> P
    end

Adds telemetry and log clients, integrating node-vtex-api with the new version of the diagnostics library that supports OpenTelemetry protocols. It also updates Typescript and Opentelemetry dependencies to enable support for node builder 6.x and IO apps using Node 16.x.

This update includes a timeout to handle clients initializing indefinitely, assuming some initial logs might be lost on application startup while the log client still loading. Now we can better handle this scenario since we have more control over the startup process of the log client. We've also updated error handling for this step, avoiding throwing errors on log client startup so the app is still available and able to use console.log fallback - ensuring the app is able to run even if the logger is not working properly.

To maintain the current standard for collecting and sending logs, we will temporarily disable the creation of this client.

src/constants.ts

src/service/logger/client.ts

src/service/logger/logger.ts

silvadenisaraujo

LGTM

daniyelnnr added the enhancement label Apr 2, 2025

daniyelnnr requested review from arturpimentel and silvadenisaraujo April 2, 2025 16:56

daniyelnnr self-assigned this Apr 2, 2025

daniyelnnr commented Apr 2, 2025

View reviewed changes

arturpimentel requested changes Apr 2, 2025

View reviewed changes

silvadenisaraujo reviewed Apr 2, 2025

View reviewed changes

silvadenisaraujo requested a review from Copilot April 2, 2025 22:06

Copilot AI reviewed Apr 2, 2025

View reviewed changes

src/service/logger/logger.ts Outdated Show resolved Hide resolved

src/caches/LRUDiskCache.ts Show resolved Hide resolved

daniyelnnr requested review from silvadenisaraujo and arturpimentel April 8, 2025 02:27

daniyelnnr added 5 commits April 16, 2025 10:00

Updates typing error handling and fix lint problems

d350670

Update logger client init parameters

e3c9caa

Disable log client initialization

470d89b

To maintain the current standard for collecting and sending logs, we will temporarily disable the creation of this client.

daniyelnnr force-pushed the update/logs-instrumentation branch from a804047 to 470d89b Compare April 16, 2025 13:03

silvadenisaraujo reviewed Apr 16, 2025

View reviewed changes

src/constants.ts Outdated Show resolved Hide resolved

src/service/logger/client.ts Show resolved Hide resolved

src/service/logger/logger.ts Show resolved Hide resolved

Rename constant adding millisecond unit

9841d3a

silvadenisaraujo approved these changes Apr 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update logging instrumentation #577

Update logging instrumentation #577

daniyelnnr commented Apr 2, 2025

daniyelnnr Apr 2, 2025 •

edited

Loading

arturpimentel Apr 2, 2025

daniyelnnr Apr 8, 2025 •

edited

Loading

arturpimentel Apr 8, 2025

daniyelnnr Apr 9, 2025

arturpimentel Apr 2, 2025

arturpimentel Apr 2, 2025

arturpimentel left a comment

silvadenisaraujo left a comment

Copilot AI left a comment

daniyelnnr commented Apr 8, 2025

silvadenisaraujo left a comment

Update logging instrumentation #577

Are you sure you want to change the base?

Update logging instrumentation #577

Conversation

daniyelnnr commented Apr 2, 2025

What is the purpose of this pull request?

What problem is this solving?

How should this be manually tested?

Types of changes

daniyelnnr Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

arturpimentel Apr 2, 2025

Choose a reason for hiding this comment

daniyelnnr Apr 8, 2025 • edited Loading

Choose a reason for hiding this comment

arturpimentel Apr 8, 2025

Choose a reason for hiding this comment

daniyelnnr Apr 9, 2025

Choose a reason for hiding this comment

arturpimentel Apr 2, 2025

Choose a reason for hiding this comment

arturpimentel Apr 2, 2025

Choose a reason for hiding this comment

arturpimentel left a comment

Choose a reason for hiding this comment

silvadenisaraujo left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

daniyelnnr commented Apr 8, 2025

Original Logging Architecture

Architecture after the changes

silvadenisaraujo left a comment

Choose a reason for hiding this comment

daniyelnnr Apr 2, 2025 •

edited

Loading

daniyelnnr Apr 8, 2025 •

edited

Loading