Skip to content

fix(sdk-node): fail-fast on LoggerProvider creation from config #6785

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
trentm merged 12 commits into from
Jun 17, 2026
Merged

fix(sdk-node): fail-fast on LoggerProvider creation from config #6785

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7032479
fix(sdk-node): fail-fast on LoggerProvider creation from config
trentm Jun 4, 2026
3c36268
add changelog entry
trentm Jun 4, 2026
aadf86d
lint:fix
trentm Jun 4, 2026
68bfbd0
drop debugging comment/message
trentm Jun 4, 2026
eff328b
add a test case for the create() throwing
trentm Jun 4, 2026
fee5125
lint:fix (prettier)
trentm Jun 5, 2026
57ba20b
Merge branch 'main' into trentm-declconf-create-play
trentm Jun 5, 2026
6b4a899
Merge branch 'main' into trentm-declconf-create-play
trentm Jun 5, 2026
8b3da47
Merge branch 'main' into trentm-declconf-create-play
trentm Jun 15, 2026
b2f95ae
move CHANGELOG entry up, since there has been a release
trentm Jun 15, 2026
4106a1e
Merge branch 'main' into trentm-declconf-create-play
trentm Jun 17, 2026
7458520
fix a 'new BatchLogRecordProcessor' usage from this PR to the new cal…
trentm Jun 17, 2026
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
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2
* fix(configuration): percent-decode keys and values in `resource.attributes_list` per spec [#6787](https://github.com/open-telemetry/opentelemetry-js/pull/6787) @MikeGoldsmith
* fix(sdk-node): apply spec-defined `schedule_delay: 1000` default for BatchLogRecordProcessor from declarative config (SDK defaults to 5000) [#6788](https://github.com/open-telemetry/opentelemetry-js/pull/6788) @MikeGoldsmith
* fix(configuration): default `log_level` to `info` in env-based config initialization for consistency with file-based config [#6788](https://github.com/open-telemetry/opentelemetry-js/pull/6788) @MikeGoldsmith
* fix(sdk-node): fail-fast on LoggerProvider creation with startNodeSDK() and declarative config [#6785](https://github.com/open-telemetry/opentelemetry-js/pull/6785) @trentm

### :books: Documentation

Expand Down
8 changes: 8 additions & 0 deletions experimental/packages/configuration/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,13 @@ export type {
HttpTls as HttpTlsConfigModel,
GrpcTls as GrpcTlsConfigModel,
SeverityNumber as SeverityNumberConfigModel,
LoggerProvider as LoggerProviderConfigModel,
AttributeLimits as AttributeLimitsConfigModel,
LogRecordProcessor as LogRecordProcessorConfigModel,
BatchLogRecordProcessor as BatchLogRecordProcessorConfigModel,
SimpleLogRecordProcessor as SimpleLogRecordProcessorConfigModel,
OtlpHttpExporter as OtlpHttpExporterConfigModel,
OtlpGrpcExporter as OtlpGrpcExporterConfigModel,
LogRecordLimits as LogRecordLimitsConfigModel,
} from './generated/types';
export { createConfigFactory } from './ConfigFactory';
135 changes: 78 additions & 57 deletions experimental/packages/opentelemetry-sdk-node/src/start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
} from '@opentelemetry/api';
import {
getInstanceID,
getLogRecordProcessorsFromConfiguration,
createLoggerProviderFromConfig,
getMeterReadersFromConfiguration,
getMeterViewsFromConfiguration,
getPropagatorFromConfiguration,
Expand All @@ -28,7 +28,6 @@ import {
} from './utils';
import { registerInstrumentations } from '@opentelemetry/instrumentation';
import type { SDKComponents, SDKOptions } from './types';
import { LoggerProvider } from '@opentelemetry/sdk-logs';
import { MeterProvider } from '@opentelemetry/sdk-metrics';
import { logs } from '@opentelemetry/api-logs';
import type {
Expand Down Expand Up @@ -83,8 +82,16 @@ export function startNodeSDK(sdkOptions?: SDKOptions): {
instrumentations: sdkOptions?.instrumentations?.flat() ?? [],
});

const components = create(config, sdkOptions);
context.setGlobalContextManager(components.contextManager);
let components: SDKComponents;
try {
components = create(config, sdkOptions);
} catch (createErr) {
diag.error(`Could not create OpenTelemetry SDK: ${createErr.message}`);
return NOOP_SDK;
}
if (components.contextManager) {
context.setGlobalContextManager(components.contextManager);
}
if (components.loggerProvider) {
logs.setGlobalLoggerProvider(components.loggerProvider);
}
Expand Down Expand Up @@ -121,63 +128,77 @@ function create(
config: ConfigurationModel,
sdkOptions?: SDKOptions
): SDKComponents {
const defaultContextManager = new AsyncLocalStorageContextManager();
defaultContextManager.enable();
const components: SDKComponents = {
contextManager: defaultContextManager,
};
const resource = setupResource(config, sdkOptions);

const propagator =
sdkOptions?.textMapPropagator === null
? null
: (sdkOptions?.textMapPropagator ??
getPropagatorFromConfiguration(config));
if (propagator) {
components.propagator = propagator;
}
const components: SDKComponents = {};

const logProcessors = getLogRecordProcessorsFromConfiguration(config);
if (logProcessors) {
const loggerProvider = new LoggerProvider({
resource: resource,
processors: logProcessors,
});
components.loggerProvider = loggerProvider;
}
try {
components.contextManager = new AsyncLocalStorageContextManager();
components.contextManager.enable();

const resource = setupResource(config, sdkOptions);

const propagator =
sdkOptions?.textMapPropagator === null
? null
: (sdkOptions?.textMapPropagator ??
getPropagatorFromConfiguration(config));
if (propagator) {
components.propagator = propagator;
}

const meterReaders = getMeterReadersFromConfiguration(config);
if (meterReaders) {
const meterViews = getMeterViewsFromConfiguration(config);
const meterProvider = new MeterProvider({
resource: resource,
readers: meterReaders,
views: meterViews ?? [],
});
components.meterProvider = meterProvider;
}
if (config.logger_provider) {
components.loggerProvider = createLoggerProviderFromConfig(
resource,
config.logger_provider,
config.attribute_limits
);
}

const spanProcessors = getSpanProcessorsFromConfiguration(config);
if (spanProcessors) {
const spanLimits = getSpanLimitsFromConfiguration(config);
// TODO (6506): support sampler configuration from config
const tracerProvider = new BasicTracerProvider({
resource,
spanProcessors,
spanLimits,
generalLimits: {
attributeValueLengthLimit:
config.attribute_limits?.attribute_value_length_limit ?? undefined,
attributeCountLimit:
config.attribute_limits?.attribute_count_limit ?? undefined,
},
// TODO (6616): support idGenerator configuration from config
// TODO (6624): support for `meterProvider: components.meterProvider`
});
components.tracerProvider = tracerProvider;
}
const meterReaders = getMeterReadersFromConfiguration(config);
if (meterReaders) {
const meterViews = getMeterViewsFromConfiguration(config);
const meterProvider = new MeterProvider({
resource: resource,
readers: meterReaders,
views: meterViews ?? [],
});
components.meterProvider = meterProvider;

@maryliag maryliag Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

to confirm (as per your PR description), do you plan on opening following PRs to update here to createMeterProviderFromConfig and below a createTracerProviderFromConfig?
if your plan is to do this right after, then fine, otherwise make sure you open an issue so is not forgotten :)

@trentm trentm Jun 15, 2026

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.

Yes, definitely. I wanted to start with just one in case discussion/review suggested significant changes.

}

const spanProcessors = getSpanProcessorsFromConfiguration(config);
if (spanProcessors) {
const spanLimits = getSpanLimitsFromConfiguration(config);
// TODO (6506): support sampler configuration from config
const tracerProvider = new BasicTracerProvider({
resource,
spanProcessors,
spanLimits,
generalLimits: {
attributeValueLengthLimit:
config.attribute_limits?.attribute_value_length_limit ?? undefined,
attributeCountLimit:
config.attribute_limits?.attribute_count_limit ?? undefined,
},
// TODO (6616): support idGenerator configuration from config
// TODO (6624): support for `meterProvider: components.meterProvider`
});
components.tracerProvider = tracerProvider;
}

return components;
return components;
} catch (createErr) {
// Clean up any SDK components that were created before the error.
if (components.loggerProvider) {
void components.loggerProvider.shutdown();
}
if (components.meterProvider) {
void components.meterProvider.shutdown();
}
if (components.tracerProvider) {
void components.tracerProvider.shutdown();
}

throw createErr;
}
}

export function setupResource(
Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-sdk-node/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export interface SDKOptions {
}

export interface SDKComponents {
contextManager: ContextManager;
contextManager?: ContextManager;

@maryliag maryliag Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

on the existing sdk start parameters (NodeSDKConfiguration) the contextManager is required, so I followed that. Any reason why now should be optional?

@trentm trentm Jun 15, 2026

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.

Making all the SDKComponents properties optional was necessary (without some coding pain) to support this change where components = {} is created outside the try { ... } so it can be used inside the catch { ... }:

  const components: SDKComponents = {};

  try {
    components.contextManager = new AsyncLocalStorageContextManager();
    components.contextManager.enable();
    ...
  } catch (err) {
     // ... use `components` here for cleanup
  }

Interface SDKComponents is only used for the return type of the internal create() function, so there is no breaking change for the user here.

loggerProvider?: LoggerProvider;
meterProvider?: MeterProvider;
tracerProvider?: BasicTracerProvider;
Expand Down
Loading