Enable telemetry control from env vars #605
Conversation
This commit creates new constants based on environment variables that will be used to proper configure diagnostics-nodejs on node-vtex-api
Make the new constants available on clients.ts file, which is responsible for create telemetry client.
This commit replaces and updates the telemetry clients to utilize the OTEL_EXPORTER_OTLP_ENDPOINT constant for exporting telemetry data
Small change on parameter to follow o11y team guidelines regarding this field
Updates parameters on telemetry client creation, making use of APPLICATION_ID and adding new additional attributes. Also, we remove unused constant CLIENT_NAME.
This update adds console logging for diagnostics configuration and checks if telemetry is enabled before registering instrumentations. It ensures that the telemetry client operates correctly based on the DIAGNOSTICS_TELEMETRY_ENABLED constant. Also, whenever the app vendor is not enabled it will use a no-op client for diagnostics, meaning any telemetry signal will be ignored
daniyelnnr
requested review from
arturpimentel,
silvadenisaraujo,
iagoaraujo,
juliobguedes and
filafb
|
Depends on #596 |
|
|
||
| export const OTEL_EXPORTER_OTLP_ENDPOINT = process.env.OTEL_EXPORTER_OTLP_ENDPOINT as string; | ||
|
|
||
| export const DK_APP_ID = process.env.NODE_VTEX_API_DK_APP_ID as string || "apps"; |
There was a problem hiding this comment.
nitpick: I would update the default name to something else to don't confuse with our Apps service.
| try { | ||
| const telemetryClient = await NewTelemetryClient( | ||
| APPLICATION_ID, | ||
| CLIENT_NAME, | ||
| DK_APP_ID, |
There was a problem hiding this comment.
Wha is the role of this DK app id here?
| if (!DIAGNOSTICS_TELEMETRY_ENABLED) { | ||
| console.warn(`Telemetry disabled for app: ${APP.ID} (vendor: ${APP.VENDOR})`); | ||
| } |
There was a problem hiding this comment.
I would go in the opposite direction here, since we're enabling the new telemetry in just a few tenants and I also prefer positive-leaning messages that assert that something is working instead of not working (which should be the default assumption). This warning could even be moved into the if (DIAGNOSTICS_TELEMETRY_ENABLED) {} block further below
| ]; | ||
|
|
||
| telemetryClient.registerInstrumentations(instrumentations); | ||
| if (DIAGNOSTICS_TELEMETRY_ENABLED) { |
There was a problem hiding this comment.
By the way, a sanity check: does this line risk disabling the current telemetry? I see this condition didn't exist before
What is the purpose of this pull request?
This pull request updates the diagnostics integration to improve configuration flexibility and control over telemetry behavior.
What problem is this solving?
The main changes include upgrading the diagnostics package, introducing new environment-based configuration options, and ensuring telemetry can be conditionally enabled or disabled.
Summary of the changes
Telemetry configuration and control:
@vtex/diagnostics-nodejsdependency to version0.1.0-io-beta.20inpackage.jsonfor latest telemetry features and bug fixes.src/constants.tsto control telemetry:OTEL_EXPORTER_OTLP_ENDPOINT(endpoint for telemetry exporter),DK_APP_ID(application ID override), andDIAGNOSTICS_TELEMETRY_ENABLED(flag to enable/disable telemetry via environment variable).Telemetry client initialization improvements:
src/service/telemetry/client.tsto use the new constants for exporter endpoint and app ID, and to pass anoopflag to disable telemetry whenDIAGNOSTICS_TELEMETRY_ENABLEDis false. Also logs a warning when telemetry is disabled. [1] [2] [3]Logger initialization:
initLogClientis always called in theLoggerconstructor insrc/service/logger/logger.ts, aligning with the updated telemetry client logic.How should this be manually tested?
Screenshots or example usage
Types of changes