-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(sdk-node,instrumentation,instrumentation-http): add declarative config support for instrumentation/development
#6864
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.
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
Closed
Closed
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
afb5d66
feat(configuration): add typed reader API for declarative config
mwear 7abfe61
feat(instrumentation): add declarative config reader to instrumentation
mwear 4e4a7e7
feat(sdk-node): Configure instrumentations from declarative config
mwear 517637b
feat(configuration): Warn on unused keys from declarative config
mwear 1cd010f
feat(instrumentation-http): implement readDeclarativeConfig override
mwear a5d051c
feat(configuration): add readConfig function to auto-warn on invalid …
mwear 5e5673a
refactor(config): Replace readConfig with wrapper passing
mwear f598345
refactor(instrumentation): Move declarativeConfigProperties to instru…
mwear 5bbea46
refactor(instrumentation): rephrase unsupported log message
mwear ce66fad
feat(sdk-node): handle instrumentations that predate declarative conf…
mwear 5638945
feat(instrumentation-http): add enable_synthetic_source_detection to …
mwear 4f36db8
feat(instrumentation): make unreadKeys recursive
mwear 9c65daf
chore(instrumentation,sdk-node): cleanup
mwear e7b40c0
docs(sdk-node,instrumentation,instrumentation-http): add changelog
mwear 3a14e24
chore(instrumentation-http): ensure to disable after each test run
mwear d8cf74a
chore(instrumentation,instrumentation-http): fix codecov underreporting
mwear File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
99 changes: 99 additions & 0 deletions
99
...kages/opentelemetry-instrumentation-http/test/functionals/http-declarative-config.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import * as assert from 'assert'; | ||
| import * as sinon from 'sinon'; | ||
| import { diag, DiagLogLevel } from '@opentelemetry/api'; | ||
| import { HttpInstrumentation } from '../../src/http'; | ||
|
|
||
| describe('HttpInstrumentation declarative config', function () { | ||
| let warn: sinon.SinonStub; | ||
| const created: HttpInstrumentation[] = []; | ||
| function makeInstrumentation( | ||
| config?: ConstructorParameters<typeof HttpInstrumentation>[0] | ||
| ): HttpInstrumentation { | ||
| const instrumentation = new HttpInstrumentation(config); | ||
| created.push(instrumentation); | ||
| return instrumentation; | ||
| } | ||
|
|
||
| beforeEach(function () { | ||
| warn = sinon.stub(); | ||
| diag.setLogger( | ||
| { | ||
| verbose: () => {}, | ||
| debug: () => {}, | ||
| info: () => {}, | ||
| warn, | ||
| error: () => {}, | ||
| }, | ||
| DiagLogLevel.WARN | ||
| ); | ||
| }); | ||
|
|
||
| afterEach(function () { | ||
| created.forEach(instrumentation => instrumentation.disable()); | ||
| created.length = 0; | ||
| diag.disable(); | ||
| sinon.restore(); | ||
| }); | ||
|
|
||
| it('maps snake_case keys to config fields', function () { | ||
| const instrumentation = makeInstrumentation(); | ||
| instrumentation.applyDeclarativeConfig({ | ||
| enabled: true, | ||
| require_parent_for_outgoing_spans: true, | ||
| server_name: 'my-server', | ||
| redacted_query_params: ['token', 'sig'], | ||
| enable_synthetic_source_detection: true, | ||
| }); | ||
|
|
||
| const config = instrumentation.getConfig(); | ||
| assert.strictEqual(config.requireParentforOutgoingSpans, true); | ||
| assert.strictEqual(config.serverName, 'my-server'); | ||
| assert.deepStrictEqual(config.redactedQueryParams, ['token', 'sig']); | ||
| assert.strictEqual(config.enableSyntheticSourceDetection, true); | ||
| sinon.assert.notCalled(warn); | ||
| }); | ||
|
|
||
| it('leaves unset fields at their existing value', function () { | ||
| const instrumentation = makeInstrumentation({ serverName: 'keep-me' }); | ||
| instrumentation.applyDeclarativeConfig({ | ||
| require_parent_for_incoming_spans: true, | ||
| }); | ||
|
|
||
| const config = instrumentation.getConfig(); | ||
| assert.strictEqual(config.requireParentforIncomingSpans, true); | ||
| assert.strictEqual(config.serverName, 'keep-me'); | ||
| }); | ||
|
|
||
| it('warns about a key it does not read', function () { | ||
| const instrumentation = makeInstrumentation(); | ||
| instrumentation.applyDeclarativeConfig({ | ||
| server_name: 'x', | ||
| not_a_real_key: true, | ||
| }); | ||
|
|
||
| sinon.assert.calledOnce(warn); | ||
| assert.match(warn.firstCall.args.join(' '), /unrecognized.*not_a_real_key/); | ||
| }); | ||
|
|
||
| it('warns on a type mismatch and keeps the default', function () { | ||
| const instrumentation = makeInstrumentation(); | ||
| instrumentation.applyDeclarativeConfig({ | ||
| require_parent_for_outgoing_spans: 'yes', | ||
| }); | ||
|
|
||
| assert.strictEqual( | ||
| instrumentation.getConfig().requireParentforOutgoingSpans, | ||
| undefined | ||
| ); | ||
| sinon.assert.calledOnce(warn); | ||
| assert.match( | ||
| warn.firstCall.args.join(' '), | ||
| /require_parent_for_outgoing_spans.*expected boolean/ | ||
| ); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
153 changes: 153 additions & 0 deletions
153
experimental/packages/opentelemetry-instrumentation/src/declarativeConfigProperties.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { diag } from '@opentelemetry/api'; | ||
|
|
||
| /** | ||
| * Typed, null-safe accessor over a parsed declarative config block. | ||
| * | ||
| * Each getter returns the typed value, or `undefined` when the key is missing or | ||
| * has the wrong type. A type mismatch is logged via `diag.warn`; a missing key | ||
| * is silent. Getters never throw. | ||
| */ | ||
| export interface DeclarativeConfigProperties { | ||
| getBoolean(key: string): boolean | undefined; | ||
| getString(key: string): string | undefined; | ||
| getNumber(key: string): number | undefined; | ||
| getStringArray(key: string): string[] | undefined; | ||
| /** | ||
| * Returns a nested accessor for an object-valued key. Repeated calls for the | ||
| * same key return the same accessor, so reads and unread-key tracking | ||
| * accumulate across calls. | ||
| */ | ||
| getStructured(key: string): DeclarativeConfigProperties | undefined; | ||
| /** | ||
| * Returns keys that no getter has read, such as typos or unsupported options. | ||
| * Recurses into structured children that were read via {@link getStructured}, | ||
| * reporting their unread keys with a dotted path (e.g. `http.client.foo`). A | ||
| * key whose nested block was never read is reported on its own. | ||
| */ | ||
| unreadKeys(): string[]; | ||
| /** | ||
| * Warn about the keys returned by {@link unreadKeys}. Convenience for callers | ||
| * that want the default warning rather than their own message. | ||
| */ | ||
| warnUnreadKeys(): void; | ||
| } | ||
|
|
||
| class DeclarativeConfigPropertiesImpl implements DeclarativeConfigProperties { | ||
| private readonly _block: Record<string, unknown>; | ||
| private readonly _read = new Set<string>(); | ||
| private readonly _children = new Map< | ||
| string, | ||
| DeclarativeConfigPropertiesImpl | ||
| >(); | ||
|
|
||
| constructor(block: Record<string, unknown>) { | ||
| this._block = block; | ||
| } | ||
|
|
||
| getBoolean(key: string): boolean | undefined { | ||
| return this._typed(key, 'boolean', v => typeof v === 'boolean'); | ||
| } | ||
|
|
||
| getString(key: string): string | undefined { | ||
| return this._typed(key, 'string', v => typeof v === 'string'); | ||
| } | ||
|
|
||
| getNumber(key: string): number | undefined { | ||
| return this._typed( | ||
| key, | ||
| 'number', | ||
| v => typeof v === 'number' && !Number.isNaN(v) | ||
| ); | ||
| } | ||
|
|
||
| getStringArray(key: string): string[] | undefined { | ||
| return this._typed( | ||
| key, | ||
| 'string array', | ||
| v => Array.isArray(v) && v.every(e => typeof e === 'string') | ||
| ); | ||
| } | ||
|
|
||
| getStructured(key: string): DeclarativeConfigProperties | undefined { | ||
| const block = this._typed( | ||
| key, | ||
| 'object', | ||
| v => typeof v === 'object' && v !== null && !Array.isArray(v) | ||
| ); | ||
| if (block === undefined) { | ||
| return undefined; | ||
| } | ||
| let child = this._children.get(key); | ||
| if (child === undefined) { | ||
| child = new DeclarativeConfigPropertiesImpl( | ||
| block as Record<string, unknown> | ||
| ); | ||
| this._children.set(key, child); | ||
| } | ||
| return child; | ||
| } | ||
|
|
||
| unreadKeys(): string[] { | ||
| const unread = Object.keys(this._block).filter(k => !this._read.has(k)); | ||
| // A child read via getStructured is itself "read", so recurse into it and | ||
| // report its unread keys with a dotted path. | ||
| for (const [key, child] of this._children) { | ||
| for (const nested of child.unreadKeys()) { | ||
| unread.push(`${key}.${nested}`); | ||
| } | ||
| } | ||
| return unread; | ||
| } | ||
|
|
||
| warnUnreadKeys(): void { | ||
| const unread = this.unreadKeys(); | ||
| if (unread.length > 0) { | ||
| diag.warn( | ||
| `ignoring unrecognized declarative config keys: ${unread.join(', ')}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Returns the value when the predicate accepts it. A missing key returns | ||
| // undefined and no warning; a present value of the wrong type returns undefined | ||
| // and a warning. Records the key as read either way. | ||
| private _typed<T>( | ||
| key: string, | ||
| expected: string, | ||
| predicate: (v: unknown) => boolean | ||
| ): T | undefined { | ||
| this._read.add(key); | ||
| const value = this._block[key]; | ||
| if (value === undefined || value === null) { | ||
| return undefined; | ||
| } | ||
| if (!predicate(value)) { | ||
| diag.warn( | ||
| `declarative config key "${key}": expected ${expected}, got ${typeof value}; ignoring` | ||
| ); | ||
| return undefined; | ||
| } | ||
| return value as T; | ||
| } | ||
| } | ||
|
|
||
| const EMPTY_BLOCK: Record<string, unknown> = {}; | ||
|
|
||
| /** | ||
| * Wrap a parsed config block in a typed accessor. A nullish or non-object block | ||
| * yields an empty accessor, so a reader never has to null-check before reading. | ||
| */ | ||
| export function declarativeConfigProperties( | ||
| block: unknown | ||
| ): DeclarativeConfigProperties { | ||
| const isObject = | ||
| typeof block === 'object' && block !== null && !Array.isArray(block); | ||
| return new DeclarativeConfigPropertiesImpl( | ||
| isObject ? (block as Record<string, unknown>) : EMPTY_BLOCK | ||
| ); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package.json test-script changes in instrumentation and instrumentation-http fix Codecov under-reporting. In a chain like
npm run test:cjs && npm run test:esm && npm run test:double-instr, each subsequent nyc run clobbers the previous run's coverage (nyc wipes .nyc_output before each run), so only the last run's coverage gets uploaded. The fix accumulates coverage across runs and emits one merged report. This should probably be split into its own PR. I included it here to get this PR green. Here is what codecov looked like without these changes: