Make snakeyaml-engine and jackson-databind optional at runtime#8270
Make snakeyaml-engine and jackson-databind optional at runtime#8270zeitlinger wants to merge 11 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8270 +/- ##
============================================
- Coverage 90.27% 90.27% -0.01%
- Complexity 7684 7685 +1
============================================
Files 850 851 +1
Lines 23186 23196 +10
Branches 2352 2352
============================================
+ Hits 20931 20940 +9
Misses 1530 1530
- Partials 725 726 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d6ba0ff to
067909e
Compare
Split DeclarativeConfiguration into two classes: - DeclarativeConfiguration: create(model) only, no YAML/jackson-databind deps - DeclarativeConfigurationParser: all parsing (MAPPER static block, parse, loadYaml, parseAndCreate, toConfigProperties, createSampler, env-var substitution, snakeyaml inner classes) Change snakeyaml-engine and jackson-databind from implementation to compileOnly in build.gradle.kts (keeping jackson-annotations as api since the generated model POJOs use @JsonProperty etc. as public API). Remove the unused jackson-dataformat-yaml dependency. Users who only call DeclarativeConfiguration.create(model) for programmatic SDK configuration no longer incur transitive YAML/jackson-databind deps. Users who need YAML parsing add snakeyaml-engine and jackson-databind and call DeclarativeConfigurationParser.parseAndCreate(stream). This also opens the door for merging declarative config into autoconfigure, since the static initializer that caused NoClassDefFoundError when jackson-databind was absent is now isolated in DeclarativeConfigurationParser. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…rser Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…module - All fileconfig source files moved from sdk-extensions/incubator to sdk-extensions/autoconfigure, removing the hard dependency on incubator for users who only need autoconfigure - New testDeclarativeConfig test suite in autoconfigure with isolated classpath so existing autoconfigure tests (which check for absent exporters) are not polluted - Updated INCUBATOR_AVAILABLE check to look for api:incubator (DeclarativeConfigException), and added separate PARSER_AVAILABLE check for snakeyaml - New sdk-extensions/autoconfigure-config convenience module that bundles autoconfigure + snakeyaml + jackson-databind for users who want file-based config out of the box - YAML test resources added to incubator/src/test/resources (ViewConfig tests still need them) Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…toconfigure module Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
- Delete fileconfig tests from src/test/ (they were moved to testDeclarativeConfig
in the previous commit but deletions were not staged)
- Fix ViewConfig/ViewConfigCustomizer javadoc: replace broken {@link} cross-module
reference to DeclarativeConfigurationParser with {@code} (class moved to
autoconfigure module which is not a main-scope dep of incubator)
- Delete incubator META-INF/services ComponentProvider registration (ServiceResourceDetector
moved to autoconfigure module)
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…o testConvertToModel - NoSharedInternalCodeTest: skip check when jar has no OpenTelemetry classes; autoconfigure-config is a dependency-only convenience module with no Java sources - api/incubator testConvertToModel: add snakeyaml-engine to runtime deps; DeclarativeConfigurationParser (now in autoconfigure) uses snakeyaml internally but autoconfigure declares it compileOnly so it doesn't flow transitively Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…urce package-private Both were public but only used within the autoconfigure module. ResourceFactory (different package, same module) now uses ResourceConfiguration.createEnvironmentResource and inlines the property name string instead. Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
…OpenTelemetry Signed-off-by: Gregor Zeitlinger <gregor.zeitlinger@grafana.com>
29a1e17 to
e3ee579
Compare
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.sdk.extension.incubator.fileconfig; |
There was a problem hiding this comment.
I don't think we're ready to stabilize these APIs quite yet. Since autoconfigure is a stable artifact, these would need to be in an internal package for the time being.
One benefit of #8265 is since its publishing to a new alpha artifact opentelemetry-sdk-extension-declarative-config, it can keep the code in a non-internal package. Later, when we're comfortable stabilizing the APIs, we can choose to either:
- Stabilize
opentelemetry-sdk-extension-declarative-config - OR, merge
opentelemetry-sdk-extension-declarative-configintoopentelemetry-sdk-extension-autoconfigure, keeping the package identical.
I like the sound of the second option. It lands us in the same place as this PR, but minimizes churn for users.
There was a problem hiding this comment.
I don't think we're ready to stabilize these APIs quite yet.
OK - I thought this was what we're trying to do now - as you said we want to find a "permanent home".
But even if not - I think it's valuable to discuss the target picture - or are we in a rush to do this refactor in any way?
There was a problem hiding this comment.
OK - I thought this was what we're trying to do now
Permanent home yes, public API - I was thinking no since there are so many moving pieces just to extract that it'll be hard to focus on scrutinizing the API design as well.
But even if not - I think it's valuable to discuss the target picture
Yes definitely. Especially considering that a multi stage move (1. extract from SDK incubator 2. promote APIs to public) exposes users to more churn than one big move. Reducing the churn is why I like the second option.
There was a problem hiding this comment.
I think the same strategy could also be applied to this PR - I'd just move the files to internal in the first go.
It really boils down to: "Is it a good feature to let users skip jackson and snakeyaml".
There was a problem hiding this comment.
I'd just move the files to internal in the first go.
That's different. With #8265, we can start with a separate module opentelemetry-sdk-extension-declarative-config, and keep the code in a package in that module io.opentelemetry.sdk.autoconfigure.declarativeconfig, such that we could later merge the new module into opentelemetry-sdk-extension-autoconfigure without any change to the package.
Not *.internal.declarativeconfig -> *.declarativeconfig transition required for users. Just change dependency from opentelemetry-sdk-extension-declarative-config to opentelemetry-sdk-extension-autoconfigure.
It really boils down to: "Is it a good feature to let users skip jackson and snakeyaml".
I do think its a good idea. I would go with a pluggable design with SPI defining the contract for parsing YAML, and snakeyaml + jackson just one implementation.
There was a problem hiding this comment.
got it - so we keep this PR as a follow up then (even if we re-create it then)
|
In #8265 description I proposed followup work to make YAML parsing pluggable:
User facing difference between this and #8265: With this, users depend on |
Summary
This PR does two things:
1. Split YAML parsing out of
DeclarativeConfigurationDeclarativeConfigurationis split into two classes to make YAML dependencies optional at runtime:DeclarativeConfiguration—create(model)methods only. No dependency onsnakeyaml-engineorjackson-databindat runtime.DeclarativeConfigurationParser— all YAML parsing:MAPPERstatic block,parse(),loadYaml(),parseAndCreate(),toConfigProperties(), env-var substitution.The
MAPPERfield in the original class is astatic finalwith a static initializer. Loading the class to callcreate(model)triggered that static block, failing withNoClassDefFoundErrorifjackson-databindwas absent. Moving the static block toDeclarativeConfigurationParserbreaks the link — the JVM only loads it when parser methods are actually called.2. Move all declarative config (fileconfig) from
incubatorintoautoconfigureAll ~45
fileconfigsource files are moved fromsdk-extensions/incubatortosdk-extensions/autoconfigure. This is the direction suggested in #8265 — the YAML dep concern is resolved by the class split above.A new
sdk-extensions/autoconfigure-configconvenience module bundlesautoconfigure+snakeyaml-engine+jackson-databindfor users who want everything in one dependency (Spring Boot starter style).Dependency graph
graph TD config["autoconfigure-config\n(convenience bundle)"] autoconfigure["autoconfigure\n(fileconfig included)"] incubator["incubator\n(ViewConfig, SPI extensions)"] api-incubator["api:incubator"] snakeyaml["snakeyaml-engine"] jackson["jackson-databind"] jackson-ann["jackson-annotations"] config -->|api| autoconfigure config -->|runtimeOnly| snakeyaml config -->|runtimeOnly| jackson autoconfigure -->|api| jackson-ann autoconfigure -.->|"compileOnly (YAML parsing)"| snakeyaml autoconfigure -.->|"compileOnly (YAML parsing)"| jackson autoconfigure -.->|"compileOnly (fileconfig)"| api-incubator autoconfigure -.->|"compileOnly (optional SPI)"| incubator incubator -->|api| api-incubatorDashed arrows =
compileOnly/ optional at runtime. YAML file config requires snakeyaml + jackson + api:incubator at runtime;autoconfigure-configprovides all three.What changes for users
AutoConfiguredOpenTelemetrySdk(env/props only)autoconfigureautoconfigure(unchanged)otel.config.file)autoconfigure+incubator+ snakeyaml + jacksonautoconfigure-configDeclarativeConfiguration.create(model)incubator+ snakeyaml + jackson (YAML deps leaked)autoconfigureonlyautoconfigure+incubator+ api:incubatorRelated to / supersedes the direction in #8265.