Skip to content
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.

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

chore(otel-node): add env var parser #442

Merged
merged 5 commits into from
Nov 21, 2024
Merged

Conversation

david-luna
Copy link
Member

Upgrades the getEnvVar function:

  • return values are parsed and properly typed
  • if not defined returns the default value as per specs

It also removes the code meant to override the user-agent header of the exporters since is not possible anymore after the refactoring of exporters, with breaking changes, in several PRs

Fixes: #441

@david-luna david-luna requested a review from trentm November 20, 2024 17:41
packages/opentelemetry-node/lib/environment.js Outdated Show resolved Hide resolved
packages/opentelemetry-node/lib/environment.js Outdated Show resolved Hide resolved
@@ -64,6 +64,7 @@
},
"types": "types/index.d.ts",
"dependencies": {
"@opentelemetry/core": "1.27.0",
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a caret dep, like the others? Also, FWIW I notice that the ".../resources" dep below is ^1.26.0. I don't think that matters. It depends on the package-lock.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was hesitating but I'm going to add it

* @property {number} OTEL_METRIC_EXPORT_TIMEOUT
* @property {boolean} ELASTIC_OTEL_METRICS_DISABLED
*/
const otelEnv = getEnv();
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, there was discussion about getting rid of this getEnv() facility. Not soon, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the criteria of open-telemetry/opentelemetry-js#5172 getEnv is used in several packages so I thought it was safe. I'll add a note. Thanks for the heads up :)

Copy link
Member

Choose a reason for hiding this comment

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

Yah, should be fine. Just noting. getEnv() might survive for a long while.

*/
export function getEnvVar(name: string): string | undefined;
export function getEnvVar<T extends "OTEL_TRACES_EXPORTER" | "OTEL_LOG_LEVEL" | "OTEL_SDK_DISABLED" | "OTEL_BSP_EXPORT_TIMEOUT" | "OTEL_BSP_MAX_EXPORT_BATCH_SIZE" | "OTEL_BSP_MAX_QUEUE_SIZE" | "OTEL_BSP_SCHEDULE_DELAY" | "OTEL_BLRP_EXPORT_TIMEOUT" | "OTEL_BLRP_MAX_EXPORT_BATCH_SIZE" | "OTEL_BLRP_MAX_QUEUE_SIZE" | "OTEL_BLRP_SCHEDULE_DELAY" | "OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT" | "OTEL_ATTRIBUTE_COUNT_LIMIT" | "OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT" | "OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT" | "OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT" | "OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT" | "OTEL_SPAN_EVENT_COUNT_LIMIT" | "OTEL_SPAN_LINK_COUNT_LIMIT" | "OTEL_SPAN_ATTRIBUTE_PER_EVENT_COUNT_LIMIT" | "OTEL_SPAN_ATTRIBUTE_PER_LINK_COUNT_LIMIT" | "OTEL_EXPORTER_OTLP_TIMEOUT" | "OTEL_EXPORTER_OTLP_TRACES_TIMEOUT" | "OTEL_EXPORTER_OTLP_METRICS_TIMEOUT" | "OTEL_EXPORTER_OTLP_LOGS_TIMEOUT" | "OTEL_EXPORTER_JAEGER_AGENT_PORT" | "OTEL_NO_PATCH_MODULES" | "OTEL_PROPAGATORS" | "OTEL_SEMCONV_STABILITY_OPT_IN" | "CONTAINER_NAME" | "ECS_CONTAINER_METADATA_URI_V4" | "ECS_CONTAINER_METADATA_URI" | "HOSTNAME" | "KUBERNETES_SERVICE_HOST" | "NAMESPACE" | "OTEL_EXPORTER_JAEGER_AGENT_HOST" | "OTEL_EXPORTER_JAEGER_ENDPOINT" | "OTEL_EXPORTER_JAEGER_PASSWORD" | "OTEL_EXPORTER_JAEGER_USER" | "OTEL_EXPORTER_OTLP_ENDPOINT" | "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT" | "OTEL_EXPORTER_OTLP_METRICS_ENDPOINT" | "OTEL_EXPORTER_OTLP_LOGS_ENDPOINT" | "OTEL_EXPORTER_OTLP_HEADERS" | "OTEL_EXPORTER_OTLP_TRACES_HEADERS" | "OTEL_EXPORTER_OTLP_METRICS_HEADERS" | "OTEL_EXPORTER_OTLP_LOGS_HEADERS" | "OTEL_EXPORTER_ZIPKIN_ENDPOINT" | "OTEL_RESOURCE_ATTRIBUTES" | "OTEL_SERVICE_NAME" | "OTEL_TRACES_SAMPLER_ARG" | "OTEL_TRACES_SAMPLER" | "OTEL_LOGS_EXPORTER" | "OTEL_EXPORTER_OTLP_INSECURE" | "OTEL_EXPORTER_OTLP_TRACES_INSECURE" | "OTEL_EXPORTER_OTLP_METRICS_INSECURE" | "OTEL_EXPORTER_OTLP_LOGS_INSECURE" | "OTEL_EXPORTER_OTLP_CERTIFICATE" | "OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE" | "OTEL_EXPORTER_OTLP_METRICS_CERTIFICATE" | "OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE" | "OTEL_EXPORTER_OTLP_COMPRESSION" | "OTEL_EXPORTER_OTLP_TRACES_COMPRESSION" | "OTEL_EXPORTER_OTLP_METRICS_COMPRESSION" | "OTEL_EXPORTER_OTLP_LOGS_COMPRESSION" | "OTEL_EXPORTER_OTLP_CLIENT_KEY" | "OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY" | "OTEL_EXPORTER_OTLP_METRICS_CLIENT_KEY" | "OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY" | "OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE" | "OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE" | "OTEL_EXPORTER_OTLP_METRICS_CLIENT_CERTIFICATE" | "OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE" | "OTEL_EXPORTER_OTLP_PROTOCOL" | "OTEL_EXPORTER_OTLP_TRACES_PROTOCOL" | "OTEL_EXPORTER_OTLP_METRICS_PROTOCOL" | "OTEL_EXPORTER_OTLP_LOGS_PROTOCOL" | "OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE" | keyof EdotEnv>(name: T): T extends "OTEL_TRACES_EXPORTER" | "OTEL_LOG_LEVEL" | "OTEL_SDK_DISABLED" | "OTEL_BSP_EXPORT_TIMEOUT" | "OTEL_BSP_MAX_EXPORT_BATCH_SIZE" | "OTEL_BSP_MAX_QUEUE_SIZE" | "OTEL_BSP_SCHEDULE_DELAY" | "OTEL_BLRP_EXPORT_TIMEOUT" | "OTEL_BLRP_MAX_EXPORT_BATCH_SIZE" | "OTEL_BLRP_MAX_QUEUE_SIZE" | "OTEL_BLRP_SCHEDULE_DELAY" | "OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT" | "OTEL_ATTRIBUTE_COUNT_LIMIT" | "OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT" | "OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT" | "OTEL_LOGRECORD_ATTRIBUTE_VALUE_LENGTH_LIMIT" | "OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT" | "OTEL_SPAN_EVENT_COUNT_LIMIT" | "OTEL_SPAN_LINK_COUNT_LIMIT" | "OTEL_SPAN_ATTRIBUTE_PER_EVENT_COUNT_LIMIT" | "OTEL_SPAN_ATTRIBUTE_PER_LINK_COUNT_LIMIT" | "OTEL_EXPORTER_OTLP_TIMEOUT" | "OTEL_EXPORTER_OTLP_TRACES_TIMEOUT" | "OTEL_EXPORTER_OTLP_METRICS_TIMEOUT" | "OTEL_EXPORTER_OTLP_LOGS_TIMEOUT" | "OTEL_EXPORTER_JAEGER_AGENT_PORT" | "OTEL_NO_PATCH_MODULES" | "OTEL_PROPAGATORS" | "OTEL_SEMCONV_STABILITY_OPT_IN" | "CONTAINER_NAME" | "ECS_CONTAINER_METADATA_URI_V4" | "ECS_CONTAINER_METADATA_URI" | "HOSTNAME" | "KUBERNETES_SERVICE_HOST" | "NAMESPACE" | "OTEL_EXPORTER_JAEGER_AGENT_HOST" | "OTEL_EXPORTER_JAEGER_ENDPOINT" | "OTEL_EXPORTER_JAEGER_PASSWORD" | "OTEL_EXPORTER_JAEGER_USER" | "OTEL_EXPORTER_OTLP_ENDPOINT" | "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT" | "OTEL_EXPORTER_OTLP_METRICS_ENDPOINT" | "OTEL_EXPORTER_OTLP_LOGS_ENDPOINT" | "OTEL_EXPORTER_OTLP_HEADERS" | "OTEL_EXPORTER_OTLP_TRACES_HEADERS" | "OTEL_EXPORTER_OTLP_METRICS_HEADERS" | "OTEL_EXPORTER_OTLP_LOGS_HEADERS" | "OTEL_EXPORTER_ZIPKIN_ENDPOINT" | "OTEL_RESOURCE_ATTRIBUTES" | "OTEL_SERVICE_NAME" | "OTEL_TRACES_SAMPLER_ARG" | "OTEL_TRACES_SAMPLER" | "OTEL_LOGS_EXPORTER" | "OTEL_EXPORTER_OTLP_INSECURE" | "OTEL_EXPORTER_OTLP_TRACES_INSECURE" | "OTEL_EXPORTER_OTLP_METRICS_INSECURE" | "OTEL_EXPORTER_OTLP_LOGS_INSECURE" | "OTEL_EXPORTER_OTLP_CERTIFICATE" | "OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE" | "OTEL_EXPORTER_OTLP_METRICS_CERTIFICATE" | "OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE" | "OTEL_EXPORTER_OTLP_COMPRESSION" | "OTEL_EXPORTER_OTLP_TRACES_COMPRESSION" | "OTEL_EXPORTER_OTLP_METRICS_COMPRESSION" | "OTEL_EXPORTER_OTLP_LOGS_COMPRESSION" | "OTEL_EXPORTER_OTLP_CLIENT_KEY" | "OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY" | "OTEL_EXPORTER_OTLP_METRICS_CLIENT_KEY" | "OTEL_EXPORTER_OTLP_LOGS_CLIENT_KEY" | "OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE" | "OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE" | "OTEL_EXPORTER_OTLP_METRICS_CLIENT_CERTIFICATE" | "OTEL_EXPORTER_OTLP_LOGS_CLIENT_CERTIFICATE" | "OTEL_EXPORTER_OTLP_PROTOCOL" | "OTEL_EXPORTER_OTLP_TRACES_PROTOCOL" | "OTEL_EXPORTER_OTLP_METRICS_PROTOCOL" | "OTEL_EXPORTER_OTLP_LOGS_PROTOCOL" | "OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE" ? import("@opentelemetry/core").ENVIRONMENT[T] : EdotEnv[T];
Copy link
Member

Choose a reason for hiding this comment

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

This is big. Should that be concerning?

Copy link
Member Author

Choose a reason for hiding this comment

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

TS is expanding the type keyof ENVIRONMENT. It shouldn't be a problem since this will be kept updated by running npm run gen:types. I'll check if there is a way to tell TS to make it shorter

Copy link
Member Author

Choose a reason for hiding this comment

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

Did some type gymnastics to avoid the key expansion without luck. The way it could be fixed is to update TS, I've tried with latest v5.6.3 and it works. Maybe we should consider updating it (in a future PR).

@david-luna david-luna merged commit 436940a into main Nov 21, 2024
12 checks passed
@david-luna david-luna deleted the dluna-441-env-vars-parsing branch November 21, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

environment vars handling
2 participants