feat(sdk): validate all export URLs before starting the SDK

[mquentin]

What

Following this comment of a previous and merged PR:
#346 (comment)

The SDK now validates every OTLP export URL (root and per-signal) before
starting — an invalid URL logs a diag.error and returns a no-op SDK instead of
silently dropping telemetry.

Why

Only the root URL was validated. Invalid per-signal URLs just skipped the
exporter, so with custom processors the SDK reported success while exporting
nothing. This moves the check into the SDK so the sandbox's manual guard is no
longer needed.

Changes

• core/exportUrl.ts (new): parseExportUrl() helper.
• core/sdk.ts: validate signal URLs up-front, bail to NOOP_SDK before starting
  either signal; unset URLs inherit the root endpoint.
• traces/logs SDKs: invalid URL returns NOOP_SDK instead of silently
  skipping the exporter.
• sandbox/src/otel.ts: removed the redundant guard.

[mquentin]

Second commit of the PR (refactor(sdk): drop signal-config casts in combineSdks) goal is to prevent having dangerous casting here such as

    const signalExportUrls: [string, string | undefined][] = [
      ['Logs SDK', (config?.logs as LogsConfig | undefined)?.exportConfig?.url],
      [
        'Traces SDK',
        (config?.traces as TracesConfig | undefined)?.exportConfig?.url,
      ],
    ];

I will bring the point to the SIG

[david-luna]

I don't fully get here about the dangerous casting. However I'm not opposed to use a stricter type here

The intent of ExtractConfigs<T> was to allow users of the combineSdks add extra SDKs if they se it fit. So the following code is typed

const startCustomSdk = (config: CustomConfig ) => {
  // return `WebSdk` object
}

// Make SDK composition
const startBrowserSdkWithCustom = combineSdks({
  traces: startTracesSdk,
  logs: startLogsSdk,
  custom: startCustomSdk,
});

// now the combined function has `custom` as property of the config type the user defined
startBrowserSdkWithCustom({
  serviceName: 'foo',
  traces: { /**/ },
  logs: { /**/ },
  custom: { /**/ }, // <- the editor provides type autocompletion for this
});

With the CombinedConfig we have to maintain it manually and update if necessary.

Now is see the combineSdks function is not exported. IMHO this limits the user to only the SDKs we provide here (logs, traces and both). There is always the option of telling users to create a wrapper function around the start*Sdk functions.

[david-luna]

EDIT: the new type makes sense since the combine function only looks for logs and traces properties. The logic of the method is only resolving the common config properties before calling the factory function. I think this could be done in a loop so any given SDK factory function by the user will be called too.

I guess the question we should ask is. Should we expose the combineSdks function?

[mquentin]

I don't fully get here about the dangerous casting.

I was talking about each of the config?.logs as LogsConfig

I guess the question we should ask is. Should we expose the combineSdks function?

Could be a good topic for the SIG

[david-luna]

Could be a good topic for the SIG

Added

[mquentin]

Related to the SIG discussion about enabling Traces if the log URL is the only erroneous URL and enabling the Logs if the trace URL is the only erroneous URL, it makes quite some changes in the SDK:

mquentin#1

I suggest we do not proceed with such an impactful change and keep it simple for now: if any URL is erroneous we return a fast NOOP_SDK which is the recommended solution of Trent Mick

[david-luna]

Related to the SIG discussion about enabling Traces if the log URL is the only erroneous URL and enabling the Logs if the trace URL is the only erroneous URL, it makes quite some changes in the SDK:

mquentin#1

I suggest we do not proceed with such an impactful change and keep it simple for now: if any URL is erroneous we return a fast NOOP_SDK which is the recommended solution of Trent Mick

For completeness I would add this aligns with the NodeSDK behavior with declarative configuration. I also think is a good idea to behave the same way so we do not confuse dev that instrument both frontend and backend

[david-luna]

LGTM. I'd wait a bit for other approvers to give feedback befor merge.

[mquentin]

So @overbalance to address both of you feedbacks I introduced two commits:

• d449831 - that address the doc miss alignment
• b30514b - that make sure we can now differentiate the NOOP_SDKs from the invalid SDKs and then, later like in the sandbox example prevent registering any instrumentations

[david-luna]

@mquentin could you fix the merge conflict?

[mquentin]

@mquentin could you fix the merge conflict?

@david-luna sorry for the delay, here is the fix pushed

[joaquin-diaz]

Do we think that in the future we might have something else triggering a failure in SDK start that is not config related? I can't think of an example right now but I'm wondering if we would cage ourselves with this name and force us to change it in the future or add another flag for a different kind of start up failure

Maybe we can rename it something more generic like startFailed or just failed