feat(sdk-node,instrumentation,instrumentation-http): add declarative config support for instrumentation/development#6864
feat(sdk-node,instrumentation,instrumentation-http): add declarative config support for instrumentation/development#6864mwear wants to merge 16 commits into
instrumentation/development#6864Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6864 +/- ##
==========================================
- Coverage 95.58% 95.16% -0.42%
==========================================
Files 388 398 +10
Lines 13152 14340 +1188
Branches 3001 3318 +317
==========================================
+ Hits 12571 13647 +1076
- Misses 581 693 +112
🚀 New features to boost your workflow:
|
| "test:cjs": "nyc --silent mocha test/**/*.test.ts", | ||
| "test:esm": "nyc --silent --no-clean node --experimental-loader=@opentelemetry/instrumentation/hook.mjs ../../../node_modules/mocha/bin/mocha 'test/**/*.test.mjs'", | ||
| "test:double-instr": "nyc --silent --no-clean node --experimental-loader=@opentelemetry/instrumentation/hook.mjs ../../../node_modules/mocha/bin/mocha 'test/integrations/double-instr.test.cjs'", | ||
| "test": "npm run test:cjs && npm run test:esm && npm run test:double-instr && nyc report", |
There was a problem hiding this comment.
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:
|
This PR has review comments. Review suggestions, whether from maintainers or automated reviewers, aren't always correct or required. Please evaluate each comment on its merits, then make sure each thread has a clear outcome. For example, link to the commit if you applied a suggestion, explain why it wasn't applied, or ask a follow-up question. Automation flags a PR for human review once every review thread has a reply or is marked as resolved. Status across open PRs is visible on the pull request dashboard. |
|
Thanks, Matt. My understanding of the intent of the spec (https://opentelemetry.io/docs/specs/otel/configuration/api/) was that we would add a ConfigProvider to or something like that, and then use settings on that (E.g. in Java: https://github.com/open-telemetry/opentelemetry-java/blob/main/api/incubator/src/main/java/io/opentelemetry/api/incubator/config/ConfigProvider.java#L20-L29 and a usage example https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/3118b49eade43b82bac593a980cb83db1ee540b1/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java#L400-L418) This would require some sequencing. I think, then, we'd be able to get away from having to add methods to the InstrumentationBase class. This is all vapourware, however. |
|
@trentm, Thanks for the overview of how things should work. I somehow missed that. I'll rework this to follow the ConfigProvider approach and see how it works out. |
|
I've looked things over and the good news is that most of this is reusable. The ConfigProvider approach is a pull model, whereas the readers approach is push model from the instrumentation perspective. The biggest change is introducing a ConfigProvider API to pull from. I'll close this PR and open a new one when I have that working. |
Which problem is this PR solving?
This PR adds support for the
instrumentation/developmentsection of declarative config. It enables instrumentations to read and apply config from that section and updates thehttpinstrumentation to use it as an example.The approach is opt-in typed readers rather than a generic passthrough: each instrumentation maps the snake_case keys it supports onto its config fields. Invalid types and keys are detected and logged as warnings. The implementation is tolerant of instrumentations that have yet to implement support for declarative config or that have been built against an older
@opentelemetry/instrumentationbase.The
generalsection (shared semconv config) is intentionally left for a follow-up; the reader signature already accepts it.Fixes #6854
Short description of the changes
@opentelemetry/instrumentation:applyDeclarativeConfig(block, general?)toInstrumentationBaseand an overridablereadDeclarativeConfig(). The default readsenabled; an instrumentation overrides the reader to map its own keys.declarativeConfigProperties(), a null-safe typed accessor over a config block (getBoolean/getString/getNumber/getStringArray/getStructured).@opentelemetry/sdk-node:startNodeSDK()matches each instrumentation to itsinstrumentation/development.jsblock by package name and applies it, gating registration onenabled.@opentelemetry/instrumentationbase (noapplyDeclarativeConfig): honorenableddirectly and warn that field config needs an update.@opentelemetry/instrumentation-http:headersToSpanAttributesis deferred withgeneral, since it is the same concept asgeneral.http.*.captured_headers.An example config file:
A note on validation: unknown keys in an instrumentation block are warned about and ignored, not rejected. The schema-validated sections (
tracer_provider, etc.) are strict via the config model, but the per-instrumentation block is an open map in the schema, so the reader is the only validator. Java, the only existing implementation, behaves the same way, so this keeps us consistent while keeping a config typo from breaking startup.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: