From 4ec8cf70655e8df0452d5a9ef43a55ada7c4d254 Mon Sep 17 00:00:00 2001 From: Daniel Climent Date: Fri, 20 Oct 2023 10:26:18 +0200 Subject: [PATCH 1/7] feat: add support for loading a tracing baggage dump in wc3 baggage format --- package-lock.json | 23 +++++++++ packages/build/package.json | 1 + packages/build/src/tracing/main.ts | 18 +++++++ packages/build/tests/tracing/tests.js | 70 ++++++++++++++++++++++++++- 4 files changed, 110 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index e6400526d4..a04b62f8a1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -26007,6 +26007,7 @@ "@netlify/run-utils": "^5.1.1", "@netlify/zip-it-and-ship-it": "9.25.3", "@opentelemetry/api": "^1.4.1", + "@opentelemetry/core": "^1.17.1", "@sindresorhus/slugify": "^2.0.0", "ansi-escapes": "^6.0.0", "chalk": "^5.0.0", @@ -26305,6 +26306,28 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "packages/build/node_modules/@opentelemetry/core": { + "version": "1.17.1", + "resolved": "https://registry.npmjs.org/@opentelemetry/core/-/core-1.17.1.tgz", + "integrity": "sha512-I6LrZvl1FF97FQXPR0iieWQmKnGxYtMbWA1GrAXnLUR+B1Hn2m8KqQNEIlZAucyv00GBgpWkpllmULmZfG8P3g==", + "dependencies": { + "@opentelemetry/semantic-conventions": "1.17.1" + }, + "engines": { + "node": ">=14" + }, + "peerDependencies": { + "@opentelemetry/api": ">=1.0.0 <1.7.0" + } + }, + "packages/build/node_modules/@opentelemetry/semantic-conventions": { + "version": "1.17.1", + "resolved": "https://registry.npmjs.org/@opentelemetry/semantic-conventions/-/semantic-conventions-1.17.1.tgz", + "integrity": "sha512-xbR2U+2YjauIuo42qmE8XyJK6dYeRMLJuOlUP5SO4auET4VtOHOzgkRVOq+Ik18N+Xf3YPcqJs9dZMiDddz1eQ==", + "engines": { + "node": ">=14" + } + }, "packages/build/node_modules/chalk": { "version": "5.3.0", "resolved": "https://registry.npmjs.org/chalk/-/chalk-5.3.0.tgz", diff --git a/packages/build/package.json b/packages/build/package.json index bf0dadc709..79b0793a71 100644 --- a/packages/build/package.json +++ b/packages/build/package.json @@ -76,6 +76,7 @@ "@netlify/run-utils": "^5.1.1", "@netlify/zip-it-and-ship-it": "9.25.3", "@opentelemetry/api": "^1.4.1", + "@opentelemetry/core": "^1.17.1", "@sindresorhus/slugify": "^2.0.0", "ansi-escapes": "^6.0.0", "chalk": "^5.0.0", diff --git a/packages/build/src/tracing/main.ts b/packages/build/src/tracing/main.ts index c15fad64c8..f00191adc5 100644 --- a/packages/build/src/tracing/main.ts +++ b/packages/build/src/tracing/main.ts @@ -1,5 +1,8 @@ +import { readFile } from 'node:fs/promises' + import { HoneycombSDK } from '@honeycombio/opentelemetry-node' import { context, trace, propagation, SpanStatusCode, diag, DiagLogLevel, DiagLogger } from '@opentelemetry/api' +import { parseKeyPairsIntoRecord } from '@opentelemetry/core/build/src/baggage/utils.js' import { NodeSDK } from '@opentelemetry/sdk-node' import type { TracingOptions } from '../core/types.js' @@ -74,6 +77,7 @@ export const setMultiSpanAttributes = function (attributes: { [key: string]: str Object.entries(attributes).forEach((entry) => { baggage = baggage.setEntry(entry[0], { value: entry[1] }) }) + return propagation.setBaggage(context.active(), baggage) } @@ -89,6 +93,20 @@ export const addErrorToActiveSpan = function (error: Error) { }) } +//** Loads the baggage attributes from a baggabe file which follows W3C Baggage specification */ +export const loadBaggageFromFile = async function (baggageFilePath: string) { + if (baggageFilePath === undefined || baggageFilePath.length == 0) return setMultiSpanAttributes({}) + let baggageString: string + try { + baggageString = await readFile(baggageFilePath, 'utf-8') + } catch (error) { + diag.error(error) + return + } + const parsedBaggage = parseKeyPairsIntoRecord(baggageString) + return setMultiSpanAttributes(parsedBaggage) +} + /** Attributes used for the root span of our execution */ export type RootExecutionAttributes = { 'build.id': string diff --git a/packages/build/tests/tracing/tests.js b/packages/build/tests/tracing/tests.js index 01d43fbd03..e6fff429f2 100644 --- a/packages/build/tests/tracing/tests.js +++ b/packages/build/tests/tracing/tests.js @@ -1,8 +1,14 @@ -import { trace, TraceFlags } from '@opentelemetry/api' +import { writeFile, mkdir, rm } from 'fs/promises' +import { fileURLToPath } from 'url' + +import { trace, TraceFlags, propagation } from '@opentelemetry/api' import { getBaggage } from '@opentelemetry/api/build/src/baggage/context-helpers.js' import test from 'ava' -import { setMultiSpanAttributes, startTracing, stopTracing } from '../../lib/tracing/main.js' +import { setMultiSpanAttributes, startTracing, stopTracing, loadBaggageFromFile } from '../../lib/tracing/main.js' + +const FIXTURES_DIR = fileURLToPath(new URL('fixtures', import.meta.url)) +const BAGGAGE_PATH = `${FIXTURES_DIR}/baggage.dump` test('Tracing set multi span attributes', async (t) => { const ctx = setMultiSpanAttributes({ some: 'test', foo: 'bar' }) @@ -11,6 +17,66 @@ test('Tracing set multi span attributes', async (t) => { t.is(baggage.getEntry('foo').value, 'bar') }) +const testMatrixBaggageFile = [ + { + description: 'when baggageFilePath is blank', + input: { + baggageFilePath: '', + baggageFileContent: null, + }, + expects: { + somefield: undefined, + foo: undefined, + }, + }, + { + description: 'when baggageFilePath is set but file is empty', + input: { + baggageFilePath: BAGGAGE_PATH, + baggageFileContent: '', + }, + expects: { + somefield: undefined, + foo: undefined, + }, + }, + { + description: 'when baggageFilePath is set and has content', + input: { + baggageFilePath: BAGGAGE_PATH, + baggageFileContent: 'somefield=value,foo=bar', + }, + expects: { + somefield: { value: 'value' }, + foo: { value: 'bar' }, + }, + }, +] + +testMatrixBaggageFile.forEach((testCase) => { + test.serial(`Tracing baggage loading - ${testCase.description}`, async (t) => { + const { input, expects } = testCase + if (input.baggageFilePath.length > 0) { + await mkdir(FIXTURES_DIR, { recursive: true }) + await writeFile(input.baggageFilePath, input.baggageFileContent) + } + + const ctx = await loadBaggageFromFile(input.baggageFilePath) + const baggage = propagation.getBaggage(ctx) + + Object.entries(expects).forEach(([property, expected]) => { + if (expected === undefined) { + t.is(baggage.getEntry(property), expected) + } else { + t.is(baggage.getEntry(property).value, expected.value) + } + }) + if (input.baggageFilePath.length > 0) { + rm(input.baggageFilePath, { force: true }) + } + }) +}) + const spanId = '6e0c63257de34c92' // The sampler is deterministic, meaning that for a given traceId it will always produce a `SAMPLED` or a `NONE` // consistently. More info in - https://opentelemetry.io/docs/specs/otel/trace/tracestate-probability-sampling/#consistent-probability-sampling From 7bf5ee97d4261dd8f9c067b66c96c06723014279 Mon Sep 17 00:00:00 2001 From: Daniel Climent Date: Fri, 20 Oct 2023 12:01:30 +0200 Subject: [PATCH 2/7] feat: expose flag to set baggageFilePath --- packages/build/src/core/flags.js | 5 +++++ packages/build/src/core/normalize_flags.ts | 1 + packages/build/src/core/types.ts | 1 + packages/build/src/tracing/main.ts | 7 ++++++- 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/build/src/core/flags.js b/packages/build/src/core/flags.js index 6f9521345f..268ae93eac 100644 --- a/packages/build/src/core/flags.js +++ b/packages/build/src/core/flags.js @@ -244,6 +244,11 @@ Default: false`, describe: 'Trace flags containing the trace settings for the given trace ID', hidden: true, }, + 'tracing.baggageFilePath': { + string: true, + describe: '', + hidden: true, + }, offline: { boolean: true, describe: `Do not send requests to the Netlify API to retrieve site settings. diff --git a/packages/build/src/core/normalize_flags.ts b/packages/build/src/core/normalize_flags.ts index 6b9925d326..e736f60f6c 100644 --- a/packages/build/src/core/normalize_flags.ts +++ b/packages/build/src/core/normalize_flags.ts @@ -106,6 +106,7 @@ const getDefaultFlags = function ({ env: envOpt = {} }, combinedEnv) { sampleRate: 1, httpProtocol: DEFAULT_OTEL_ENDPOINT_PROTOCOL, port: DEFAULT_OTEL_TRACING_PORT, + baggageFilePath: '', }, timeline: 'build', quiet: false, diff --git a/packages/build/src/core/types.ts b/packages/build/src/core/types.ts index 3594f940fa..3df505424e 100644 --- a/packages/build/src/core/types.ts +++ b/packages/build/src/core/types.ts @@ -86,4 +86,5 @@ export type TracingOptions = { traceId: string traceFlags: number parentSpanId: string + baggageFilePath: string } diff --git a/packages/build/src/tracing/main.ts b/packages/build/src/tracing/main.ts index f00191adc5..19c20b99af 100644 --- a/packages/build/src/tracing/main.ts +++ b/packages/build/src/tracing/main.ts @@ -49,12 +49,17 @@ export const startTracing = function (options: TracingOptions, logger: (...args: // Sets the current trace ID and span ID based on the options received // this is used as a way to propagate trace context from Buildbot - return trace.setSpanContext(context.active(), { + const ctx = trace.setSpanContext(context.active(), { traceId: options.traceId, spanId: options.parentSpanId, traceFlags: options.traceFlags, isRemote: true, }) + + // Loads the contents of the passed baggageFilePath into the baggage + loadBaggageFromFile(options.baggageFilePath) + + return ctx } /** Stops the tracing service if there's one running. This will flush any ongoing events */ From 2518d5a18c94566584807d1853887f5aa6526980 Mon Sep 17 00:00:00 2001 From: Daniel Climent Date: Tue, 24 Oct 2023 12:28:18 +0200 Subject: [PATCH 3/7] fix: set ctx after loadBaggageFile --- packages/build/src/tracing/main.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/build/src/tracing/main.ts b/packages/build/src/tracing/main.ts index 19c20b99af..ecb584f8e8 100644 --- a/packages/build/src/tracing/main.ts +++ b/packages/build/src/tracing/main.ts @@ -49,7 +49,7 @@ export const startTracing = function (options: TracingOptions, logger: (...args: // Sets the current trace ID and span ID based on the options received // this is used as a way to propagate trace context from Buildbot - const ctx = trace.setSpanContext(context.active(), { + let ctx = trace.setSpanContext(context.active(), { traceId: options.traceId, spanId: options.parentSpanId, traceFlags: options.traceFlags, @@ -57,7 +57,10 @@ export const startTracing = function (options: TracingOptions, logger: (...args: }) // Loads the contents of the passed baggageFilePath into the baggage - loadBaggageFromFile(options.baggageFilePath) + loadBaggageFromFile(options.baggageFilePath).then((res) => { + if (res === undefined) return + ctx = res + }) return ctx } From c9ddd7299e6233e79ab7a507bcde3380751c36d9 Mon Sep 17 00:00:00 2001 From: JGAntunes Date: Tue, 24 Oct 2023 13:19:15 +0100 Subject: [PATCH 4/7] chore: move context around and change to readFileSync --- packages/build/src/tracing/main.ts | 31 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/packages/build/src/tracing/main.ts b/packages/build/src/tracing/main.ts index ecb584f8e8..dc41f941a3 100644 --- a/packages/build/src/tracing/main.ts +++ b/packages/build/src/tracing/main.ts @@ -1,4 +1,4 @@ -import { readFile } from 'node:fs/promises' +import { readFileSync } from 'node:fs' import { HoneycombSDK } from '@honeycombio/opentelemetry-node' import { context, trace, propagation, SpanStatusCode, diag, DiagLogLevel, DiagLogger } from '@opentelemetry/api' @@ -47,21 +47,18 @@ export const startTracing = function (options: TracingOptions, logger: (...args: sdk.start() + // Loads the contents of the passed baggageFilePath into the baggage + const baggageCtx = loadBaggageFromFile(options.baggageFilePath) + // Sets the current trace ID and span ID based on the options received // this is used as a way to propagate trace context from Buildbot - let ctx = trace.setSpanContext(context.active(), { + const ctx = trace.setSpanContext(baggageCtx, { traceId: options.traceId, spanId: options.parentSpanId, traceFlags: options.traceFlags, isRemote: true, }) - // Loads the contents of the passed baggageFilePath into the baggage - loadBaggageFromFile(options.baggageFilePath).then((res) => { - if (res === undefined) return - ctx = res - }) - return ctx } @@ -78,12 +75,13 @@ export const stopTracing = async function () { } } -/** Sets attributes to be propagated across child spans under the current context */ +/** Sets attributes to be propagated across child spans under the current active context */ export const setMultiSpanAttributes = function (attributes: { [key: string]: string }) { const currentBaggage = propagation.getBaggage(context.active()) + // Create a baggage if there's none let baggage = currentBaggage === undefined ? propagation.createBaggage() : currentBaggage - Object.entries(attributes).forEach((entry) => { - baggage = baggage.setEntry(entry[0], { value: entry[1] }) + Object.entries(attributes).forEach(([key, value]) => { + baggage = baggage.setEntry(key, { value }) }) return propagation.setBaggage(context.active(), baggage) @@ -102,14 +100,17 @@ export const addErrorToActiveSpan = function (error: Error) { } //** Loads the baggage attributes from a baggabe file which follows W3C Baggage specification */ -export const loadBaggageFromFile = async function (baggageFilePath: string) { - if (baggageFilePath === undefined || baggageFilePath.length == 0) return setMultiSpanAttributes({}) +export const loadBaggageFromFile = function (baggageFilePath: string) { + if (baggageFilePath.length === 0) { + diag.warn('Empty baggage file path provided, no context loaded') + return context.active() + } let baggageString: string try { - baggageString = await readFile(baggageFilePath, 'utf-8') + baggageString = readFileSync(baggageFilePath, 'utf-8') } catch (error) { diag.error(error) - return + return context.active() } const parsedBaggage = parseKeyPairsIntoRecord(baggageString) return setMultiSpanAttributes(parsedBaggage) From 952bc7c400b8ad3983d69f93c2dd3c0aa67c9946 Mon Sep 17 00:00:00 2001 From: Daniel Climent Date: Tue, 24 Oct 2023 14:38:17 +0200 Subject: [PATCH 5/7] fix: update tests after context changes and sync file reading --- packages/build/tests/tracing/tests.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/build/tests/tracing/tests.js b/packages/build/tests/tracing/tests.js index e6fff429f2..269ae3f4c9 100644 --- a/packages/build/tests/tracing/tests.js +++ b/packages/build/tests/tracing/tests.js @@ -61,14 +61,18 @@ testMatrixBaggageFile.forEach((testCase) => { await writeFile(input.baggageFilePath, input.baggageFileContent) } - const ctx = await loadBaggageFromFile(input.baggageFilePath) + const ctx = loadBaggageFromFile(input.baggageFilePath) const baggage = propagation.getBaggage(ctx) Object.entries(expects).forEach(([property, expected]) => { - if (expected === undefined) { - t.is(baggage.getEntry(property), expected) + if (input.baggageFilePath === '') { + t.is(baggage, undefined) } else { - t.is(baggage.getEntry(property).value, expected.value) + if (expected === undefined) { + t.is(baggage.getEntry(property), expected) + } else { + t.is(baggage.getEntry(property).value, expected.value) + } } }) if (input.baggageFilePath.length > 0) { @@ -144,6 +148,7 @@ testMatrix.forEach((testCase) => { traceId: input.traceId, traceFlags: input.traceFlags, parentSpanId: spanId, + baggageFilePath: '', }, noopLogger, ) From 29c85648418afef5e9b22bc781c482e52e61ce03 Mon Sep 17 00:00:00 2001 From: Daniel Climent Date: Wed, 25 Oct 2023 16:11:06 +0200 Subject: [PATCH 6/7] fix: update tests with PR review comments --- packages/build/tests/tracing/tests.js | 49 ++++++++++++++++----------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/packages/build/tests/tracing/tests.js b/packages/build/tests/tracing/tests.js index 269ae3f4c9..c430a1c870 100644 --- a/packages/build/tests/tracing/tests.js +++ b/packages/build/tests/tracing/tests.js @@ -1,5 +1,6 @@ -import { writeFile, mkdir, rm } from 'fs/promises' -import { fileURLToPath } from 'url' +import { writeFile, rm, mkdtemp } from 'fs/promises' +import { tmpdir } from 'os' +import { join } from 'path' import { trace, TraceFlags, propagation } from '@opentelemetry/api' import { getBaggage } from '@opentelemetry/api/build/src/baggage/context-helpers.js' @@ -7,9 +8,6 @@ import test from 'ava' import { setMultiSpanAttributes, startTracing, stopTracing, loadBaggageFromFile } from '../../lib/tracing/main.js' -const FIXTURES_DIR = fileURLToPath(new URL('fixtures', import.meta.url)) -const BAGGAGE_PATH = `${FIXTURES_DIR}/baggage.dump` - test('Tracing set multi span attributes', async (t) => { const ctx = setMultiSpanAttributes({ some: 'test', foo: 'bar' }) const baggage = getBaggage(ctx) @@ -32,7 +30,7 @@ const testMatrixBaggageFile = [ { description: 'when baggageFilePath is set but file is empty', input: { - baggageFilePath: BAGGAGE_PATH, + baggageFilePath: 'baggage.dump', baggageFileContent: '', }, expects: { @@ -43,7 +41,7 @@ const testMatrixBaggageFile = [ { description: 'when baggageFilePath is set and has content', input: { - baggageFilePath: BAGGAGE_PATH, + baggageFilePath: 'baggage.dump', baggageFileContent: 'somefield=value,foo=bar', }, expects: { @@ -53,31 +51,42 @@ const testMatrixBaggageFile = [ }, ] +let baggagePath +test.before(async () => { + baggagePath = await mkdtemp(join(tmpdir(), 'baggage-path-')) +}) + +test.after(async () => { + await rm(baggagePath, { recursive: true }) +}) + testMatrixBaggageFile.forEach((testCase) => { test.serial(`Tracing baggage loading - ${testCase.description}`, async (t) => { const { input, expects } = testCase + + // We only want to write the file if it's a non-empty string '', while we still want to test scenario + let filePath = input.baggageFilePath if (input.baggageFilePath.length > 0) { - await mkdir(FIXTURES_DIR, { recursive: true }) - await writeFile(input.baggageFilePath, input.baggageFileContent) + const filePath = `${baggagePath}/${input.baggageFilePath}` + await writeFile(filePath, input.baggageFileContent) } - const ctx = loadBaggageFromFile(input.baggageFilePath) + const ctx = loadBaggageFromFile(filePath) const baggage = propagation.getBaggage(ctx) + // When there's no file we test that baggage is not set + if (input.baggageFilePath === '') { + t.is(baggage, undefined) + return + } + Object.entries(expects).forEach(([property, expected]) => { - if (input.baggageFilePath === '') { - t.is(baggage, undefined) + if (expected === undefined) { + t.is(baggage.getEntry(property), expected) } else { - if (expected === undefined) { - t.is(baggage.getEntry(property), expected) - } else { - t.is(baggage.getEntry(property).value, expected.value) - } + t.is(baggage.getEntry(property).value, expected.value) } }) - if (input.baggageFilePath.length > 0) { - rm(input.baggageFilePath, { force: true }) - } }) }) From 500201dd09776538806e0189a519449849207408 Mon Sep 17 00:00:00 2001 From: Daniel Climent Date: Wed, 25 Oct 2023 16:26:46 +0200 Subject: [PATCH 7/7] fix: fix variable shadowing --- packages/build/tests/tracing/tests.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/build/tests/tracing/tests.js b/packages/build/tests/tracing/tests.js index c430a1c870..ac7953c42c 100644 --- a/packages/build/tests/tracing/tests.js +++ b/packages/build/tests/tracing/tests.js @@ -67,7 +67,7 @@ testMatrixBaggageFile.forEach((testCase) => { // We only want to write the file if it's a non-empty string '', while we still want to test scenario let filePath = input.baggageFilePath if (input.baggageFilePath.length > 0) { - const filePath = `${baggagePath}/${input.baggageFilePath}` + filePath = `${baggagePath}/${input.baggageFilePath}` await writeFile(filePath, input.baggageFileContent) }