fix(tracing): cleanup otel setup #5446
Conversation
|
This pull request adds or modifies JavaScript ( |
| tracing: { | ||
| describe: 'Tracing related options', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.enabled': { | ||
| boolean: true, | ||
| describe: 'Enable distributed tracing for build', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.preloadingEnabled': { | ||
| boolean: true, | ||
| describe: 'Enable distributed tracing for build via module preloading, to be removed once fully rolled out', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.apiKey': { | ||
| string: true, | ||
| describe: 'API Key for the tracing backend provider', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.httpProtocol': { | ||
| string: true, | ||
| describe: 'Traces backend protocol. HTTP or HTTPS.', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.host': { | ||
| string: true, | ||
| describe: 'Traces backend host', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.port': { | ||
| number: true, | ||
| describe: 'Traces backend port', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.traceId': { | ||
| string: true, | ||
| describe: 'Trace ID used to stitch the emited traces to', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.parentSpanId': { | ||
| string: true, | ||
| describe: 'Parent Span ID used to stitch the root parent span to', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.sampleRate': { | ||
| number: true, | ||
| describe: 'Trace sample rate for the given trace ID', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.traceFlags': { | ||
| number: true, | ||
| describe: 'Trace flags containing the trace settings for the given trace ID', | ||
| hidden: true, | ||
| }, | ||
| 'tracing.baggageFilePath': { | ||
| string: true, | ||
| describe: '', | ||
| hidden: true, | ||
| }, |
There was a problem hiding this comment.
No longer used since we moved the setup to opentelemetry-sdk-setup
|
|
||
| import { HoneycombSDK } from '@honeycombio/opentelemetry-node' |
There was a problem hiding this comment.
In this module all of the code has been moved to opentelemetry-sdk-setup
|
Looks good, but CI is complaining |
JGAntunes
force-pushed
the
chore/cleanup-otel-setup
branch
from
697c9aa
to
1e1d0e3
Compare
| @@ -672,4 +672,3 @@ test('(Proxy) agent is not passed as request option if not set', async (t) => { | |||
|
|
|||
| const TEST_RATE_LIMIT_DELAY = 5e3 | |||
| const SECS_TO_MSECS = 1e3 | |||
| /* eslint-enable max-lines */ | |||
There was a problem hiding this comment.
Had to introduce this to make eslint happy. I think has part of the lockfile updates we may have bumped the underlying eslint version and this ended up being picked up 🤷
| const benchmarkOptions: BenchmarkUserOptions = {} | ||
| // Tinybench dropped support for Node 14 recently so we need to skip benchmark tests for it | ||
| // https://github.com/tinylibs/tinybench/pull/40 and | ||
| // https://github.com/tinylibs/tinybench/pull/43 | ||
| // This is just a quick workaround to not run the bench tests in node 14 while we still support it | ||
| if (process.version.startsWith('v14')) { | ||
| benchmarkOptions.exclude = ['tests/**/*.bench.ts'] | ||
| } | ||
|
|
There was a problem hiding this comment.
With the lockfile update I've brought an update from tinybench from 2.5.0 to 2.5.1 which dropped support for node 14. We'll still run the benchmarks for the other CI test suites, but this skips it for node 14.
| const benchmarkOptions: BenchmarkUserOptions = {} | ||
| // Tinybench dropped support for Node 14 recently so we need to skip benchmark tests for it | ||
| // https://github.com/tinylibs/tinybench/pull/40 and | ||
| // https://github.com/tinylibs/tinybench/pull/43 | ||
| // This is just a quick workaround to not run the bench tests in node 14 while we still support it | ||
| if (process.version.startsWith('v14')) { | ||
| benchmarkOptions.exclude = ['tests/**/*.bench.ts'] | ||
| } | ||
|
|
There was a problem hiding this comment.
With the lockfile update I've brought an update from tinybench from 2.5.0 to 2.5.1 which dropped support for node 14. We'll still run the benchmarks for the other CI test suites, but this skips it for node 14.
| @@ -15,7 +15,7 @@ | |||
| "test": "vitest run", | |||
| "test:bench": "vitest bench", | |||
| "test:dev": "vitest", | |||
| "test:ci": "vitest run --reporter=default && vitest bench --run" | |||
| "test:ci": "vitest run --reporter=default && vitest bench --run --passWithNoTests" | |||
There was a problem hiding this comment.
In node 14 we won't run any bench tests and if tests=0 the default vitest behaviour is to exit with exit code 1
| @@ -15,7 +15,7 @@ | |||
| "test": "vitest run", | |||
| "test:bench": "vitest bench", | |||
| "test:dev": "vitest", | |||
| "test:ci": "vitest run --reporter=default && vitest bench --run" | |||
| "test:ci": "vitest run --reporter=default && vitest bench --run --passWithNoTests" | |||
There was a problem hiding this comment.
In node 14 we won't run any bench tests and if tests=0 the default vitest behaviour is to exit with exit code 1
|
Should be fixed @kitop, had to deal with a couple of CI issues 😓. Despite the low risk nature of the Otel changes, because this touches so many files (also I think build isn't a releasable state currently - https://netlify.slack.com/archives/C023PC3D08J/p1703679363161299?thread_ts=1703106655.819339&cid=C023PC3D08J) I'll look into shipping this next week. |
🎉 Thanks for submitting a pull request! 🎉
Summary
Follow up to COM-110 - https://linear.app/netlify/issue/COM-110/make-opentelemetry-an-optional-dependency-in-build - this effectively cleans up all the code duplication we've been doing by moving code to separate libraries.
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)