Skip to content

Commit

Permalink
[core-tracing] support sharing state between CJS and ESM (#28631)
Browse files Browse the repository at this point in the history
### Packages impacted by this PR

@azure/core-tracing

### Issues associated with this PR

Resolves #28619

### Describe the problem that is addressed by this PR

If you are exporting both CommonJS and ESM forms of a package, then it
is possible for both versions to be loaded at run-time.
However, the CommonJS build is a different module from the ESM build,
and thus a different thing from the point of view of the
JavaScript interpreter in Node.js.

> https://github.com/isaacs/tshy/blob/main/README.md#dual-package-hazards

tshy handles this by building programs into separate folders and treats
"dual module hazards" as a fact of life. 

One of the hazards of dual-modules is shared module-global state.

In core-tracing we have a module-global instrumenter that is used for
hooking into and lighting up OpenTelemetry based instrumentation. In order to
ensure it works in this dual-package world we must use one of multiple-recommended
workarounds.

In this case, the tshy documentation provides a solution to this with a
well-documented path forward. This is what is implemented here.

Please refer to
https://github.com/isaacs/tshy/blob/main/README.md#module-local-state
for added context
  • Loading branch information
maorleger authored Feb 22, 2024
1 parent 73ef377 commit b552c17
Show file tree
Hide file tree
Showing 9 changed files with 59 additions and 11 deletions.
4 changes: 3 additions & 1 deletion sdk/core/core-tracing/.tshy/browser.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
"../src/**/*.mts",
"../src/**/*.tsx"
],
"exclude": [],
"exclude": [
".././src/state-cjs.cts"
],
"compilerOptions": {
"outDir": "../.tshy-build/browser"
}
Expand Down
3 changes: 2 additions & 1 deletion sdk/core/core-tracing/.tshy/commonjs.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
"../src/**/*.tsx"
],
"exclude": [
"../src/**/*.mts"
"../src/**/*.mts",
"../src/state-browser.mts"
],
"compilerOptions": {
"outDir": "../.tshy-build/commonjs"
Expand Down
5 changes: 4 additions & 1 deletion sdk/core/core-tracing/.tshy/esm.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
"../src/**/*.mts",
"../src/**/*.tsx"
],
"exclude": [],
"exclude": [
".././src/state-cjs.cts",
".././src/state-browser.mts"
],
"compilerOptions": {
"outDir": "../.tshy-build/esm"
}
Expand Down
5 changes: 4 additions & 1 deletion sdk/core/core-tracing/.tshy/react-native.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
"../src/**/*.mts",
"../src/**/*.tsx"
],
"exclude": [],
"exclude": [
".././src/state-cjs.cts",
".././src/state-browser.mts"
],
"compilerOptions": {
"outDir": "../.tshy-build/react-native"
}
Expand Down
13 changes: 6 additions & 7 deletions sdk/core/core-tracing/src/instrumenter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import {
TracingContext,
TracingSpan,
} from "./interfaces.js";

import { createTracingContext } from "./tracingContext.js";
import { state } from "./state.js";

export function createDefaultTracingSpan(): TracingSpan {
return {
Expand Down Expand Up @@ -57,16 +59,13 @@ export function createDefaultInstrumenter(): Instrumenter {
};
}

/** @internal */
let instrumenterImplementation: Instrumenter | undefined;

/**
* Extends the Azure SDK with support for a given instrumenter implementation.
*
* @param instrumenter - The instrumenter implementation to use.
*/
export function useInstrumenter(instrumenter: Instrumenter): void {
instrumenterImplementation = instrumenter;
state.instrumenterImplementation = instrumenter;
}

/**
Expand All @@ -75,8 +74,8 @@ export function useInstrumenter(instrumenter: Instrumenter): void {
* @returns The currently set instrumenter
*/
export function getInstrumenter(): Instrumenter {
if (!instrumenterImplementation) {
instrumenterImplementation = createDefaultInstrumenter();
if (!state.instrumenterImplementation) {
state.instrumenterImplementation = createDefaultInstrumenter();
}
return instrumenterImplementation;
return state.instrumenterImplementation;
}
11 changes: 11 additions & 0 deletions sdk/core/core-tracing/src/state-browser.mts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { Instrumenter } from "./interfaces.js";

/**
* Browser-only implementation of the module's state. The browser esm variant will not load the commonjs state, so we do not need to share state between the two.
*/
export const state = {
instrumenterImplementation: undefined as Instrumenter | undefined,
};
11 changes: 11 additions & 0 deletions sdk/core/core-tracing/src/state-cjs.cts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

/**
* @internal
*
* Holds the singleton instrumenter, to be shared across CJS and ESM imports.
*/
export const state = {
instrumenterImplementation: undefined,
};
14 changes: 14 additions & 0 deletions sdk/core/core-tracing/src/state.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import { Instrumenter } from "./interfaces.js";
// @ts-expect-error The recommended approach to sharing module state between ESM and CJS.
// See https://github.com/isaacs/tshy/blob/main/README.md#module-local-state for additional information.
import { state as cjsState } from "../commonjs/state.js";

/**
* Defines the shared state between CJS and ESM by re-exporting the CJS state.
*/
export const state = cjsState as {
instrumenterImplementation: Instrumenter | undefined;
};
4 changes: 4 additions & 0 deletions sdk/core/core-tracing/vitest.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license.

import { defineConfig } from "vitest/config";
import { resolve } from "node:path";

export default defineConfig({
test: {
Expand All @@ -13,6 +14,9 @@ export default defineConfig({
toFake: ["setTimeout", "Date"],
},
watch: false,
alias: {
"../commonjs/state.js": resolve("./src/state-cjs.cts"),
},
include: ["test/**/*.spec.ts"],
exclude: ["test/**/browser/*.spec.ts"],
coverage: {
Expand Down

0 comments on commit b552c17

Please sign in to comment.