Skip to content

Tolerate unknown properties in resolved schema / published manifest - #1365

Open
lmolkova wants to merge 13 commits into
open-telemetry:mainfrom
lmolkova:tolerate-future
Open

Tolerate unknown properties in resolved schema / published manifest#1365
lmolkova wants to merge 13 commits into
open-telemetry:mainfrom
lmolkova:tolerate-future

Conversation

@lmolkova

@lmolkova lmolkova commented Apr 16, 2026

Copy link
Copy Markdown
Member

Older weaver can now read files written by a newer minor version.

  • Newer minor (resolved/2.X on a 2.0 build): loads, unknowns ignored, warning that newer version is available is logged.
  • Major mismatch (e.g. resolved/3.0): not supported, fatal error at start
  • Same/older minor with unknown field: fatal error listing unexpected fields (path). e.g. registry.spans[0].unknown_field

Warning

It introduces regression for definition side (we reuse quite a few types between definition and resolved).
Since deny_unknown_fields is relaxed, we'll tolerate errors in definitions that we failed on before.

Definitions PR: #1422

@codecov

codecov Bot commented Apr 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.64430% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.4%. Comparing base (7416b7e) to head (c9130ea).

Files with missing lines Patch % Lines
crates/weaver_semconv/src/manifest.rs 69.2% 4 Missing ⚠️
crates/weaver_resolver/src/loader.rs 87.5% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1365     +/-   ##
=======================================
+ Coverage   81.1%   81.4%   +0.2%     
=======================================
  Files        130     132      +2     
  Lines      11486   11608    +122     
=======================================
+ Hits        9326    9455    +129     
+ Misses      2160    2153      -7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Apr 27, 2026

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

@lmolkova
lmolkova marked this pull request as ready for review April 28, 2026 02:04
@lmolkova
lmolkova requested a review from a team as a code owner April 28, 2026 02:04
///
/// Serializes and deserializes as the string form (e.g. `"resolved/2.0"`).
#[derive(Clone, PartialEq, Eq, Hash)]
pub struct FileFormat {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lmolkova I approve of most of this PR - but wanted to check in on your plans based on the discussion today.

I'm fine leaving this here with some flexibility we discussed, or addressing file-format expansion in a later PR.

Should I approve this PR as-is?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR does not touch anything related to definition, I was going to address it in separate PR (within this release). So I think this one can go as is

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's the definition part #1422

Comment thread crates/weaver_resolver/src/loader.rs Fixed
Comment thread crates/weaver_resolver/src/loader.rs Fixed
Comment thread crates/weaver_resolver/src/loader.rs Fixed
Comment thread crates/weaver_resolver/src/loader.rs Fixed

@jsuereth jsuereth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lmolkova FYI - I forgot to approve this oh so long ago - Approving now if we still consider this a blocker - hopefully it's easy to bring this back up to date and merge if we want it.

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 18, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-08-04 14:26 UTC

Review the latest changes.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces forward-compatible parsing for resolved schema and publication manifest files by switching to a structured FileFormat (prefix/MAJOR.MINOR) and tolerating unknown fields when the minor version is newer than the build supports (while still rejecting major mismatches and unknowns on known/older minors).

Changes:

  • Add weaver_common::file_format::FileFormat plus version validation (warn on newer minor; error on prefix/major mismatch).
  • Add raw-vs-roundtripped YAML diffing (weaver_semconv::unexpected_fields) to detect and error on unknown fields for known/older minors.
  • Update resolved-schema + manifest loaders, tests, and fixtures to exercise forward-compat and typo-protection behaviors.

Reviewed changes

Copilot reviewed 42 out of 43 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/registry/package.rs Test now loads manifest.yaml via RegistryManifest::try_from_file to exercise new manifest parsing rules.
schemas/semconv.resolved.v2.json Clarifies file-format forward-compat behavior in schema description text.
schemas/publication-manifest.v2.json Clarifies file-format forward-compat behavior in schema description text.
crates/weaver_semconv/tests/published_repository/resolved/2.0.0 Fixture updated to match stricter unknown-field behavior.
crates/weaver_semconv/tests/published_repository/resolved/1.0.0 Fixture updated to match stricter unknown-field behavior.
crates/weaver_semconv/tests/published_repository/3.0.0/registry_manifest.yaml Fixture updated to match stricter unknown-field behavior.
crates/weaver_semconv/src/v2/span.rs Moves unknown-field strictness from serde to schemars for forward-compat deserialization.
crates/weaver_semconv/src/v2/mod.rs Customizes deprecated deserialization to remain forward-compatible.
crates/weaver_semconv/src/unexpected_fields.rs New module to detect unknown keys by diffing raw YAML vs normalized typed form.
crates/weaver_semconv/src/manifest.rs Manifest parsing now uses FileFormat and applies unknown-field detection for known minors.
crates/weaver_semconv/src/lib.rs Adds UnexpectedFields error variant and exports the new module.
crates/weaver_semconv/src/deprecated.rs Drops unknown keys in deprecated objects for forward compatibility.
crates/weaver_semconv/src/attribute.rs Replaces serde deny-unknown with schemars deny-unknown for schema strictness without breaking forward-compat reads.
crates/weaver_semconv_gen/src/v2.rs Updates tests to construct resolved schema file_format as FileFormat.
crates/weaver_resolver/src/registry.rs Updates expected-schema loading to go through new from_yaml_value path.
crates/weaver_resolver/src/loader.rs Loads resolved schemas as YAML Value then validates via V2Schema::from_yaml_value; adds forward-compat/typo-protection tests.
crates/weaver_resolver/src/lib.rs Updates V1 resolved schema file_format to use V1_RESOLVED_FILE_FORMAT and uses from_yaml_value in tests.
crates/weaver_resolver/src/dependency.rs Updates test schema construction to use parsed FileFormat.
crates/weaver_resolver/src/attribute.rs Updates test schema construction to use parsed FileFormat.
crates/weaver_resolver/data/registry-test-resolved-unknown-field/published/resolved_schema.yaml New fixture: known-minor resolved schema with seeded typos for unknown-field rejection tests.
crates/weaver_resolver/data/registry-test-resolved-unknown-field/published/manifest.yaml New fixture: manifest referencing the typo-seeded resolved schema.
crates/weaver_resolver/data/registry-test-resolved-minor-ahead/published/resolved_schema.yaml New fixture: future-minor resolved schema with unknown fields for forward-compat acceptance tests.
crates/weaver_resolver/data/registry-test-resolved-minor-ahead/published/manifest.yaml New fixture: future-minor manifest with unknown fields for forward-compat acceptance tests.
crates/weaver_resolver/data/registry-test-resolved-major-mismatch/published/resolved_schema.yaml New fixture: major-mismatch resolved schema for fatal rejection test.
crates/weaver_resolver/data/registry-test-resolved-major-mismatch/published/manifest.yaml New fixture: manifest referencing a major-mismatch resolved schema for fatal rejection test.
crates/weaver_resolver/data/registry-test-manifest-unknown-field/published/resolved_schema.yaml New fixture: clean resolved schema used to isolate manifest unknown-field rejection.
crates/weaver_resolver/data/registry-test-manifest-unknown-field/published/manifest.yaml New fixture: known-minor manifest with seeded unknown fields for rejection test.
crates/weaver_resolved_schema/src/v2/span.rs Switches to schemars deny-unknown to keep JSON schema strict while allowing forward-compat deserialization.
crates/weaver_resolved_schema/src/v2/registry.rs Same schemars/serde unknown-field behavior adjustment for forward-compat.
crates/weaver_resolved_schema/src/v2/refinements.rs Same schemars/serde unknown-field behavior adjustment for forward-compat.
crates/weaver_resolved_schema/src/v2/mod.rs Changes file_format to FileFormat, introduces from_yaml_value, and adds extensive tests for version/unknown-field behavior.
crates/weaver_resolved_schema/src/v2/metric.rs Same schemars/serde unknown-field behavior adjustment for forward-compat.
crates/weaver_resolved_schema/src/v2/event.rs Same schemars/serde unknown-field behavior adjustment for forward-compat.
crates/weaver_resolved_schema/src/v2/entity.rs Same schemars/serde unknown-field behavior adjustment for forward-compat.
crates/weaver_resolved_schema/src/v2/attribute.rs Same schemars/serde unknown-field behavior adjustment for forward-compat.
crates/weaver_resolved_schema/src/v2/attribute_group.rs Same schemars/serde unknown-field behavior adjustment for forward-compat.
crates/weaver_resolved_schema/src/lib.rs Promotes V1/V2 file format constants to FileFormat and updates struct field types.
crates/weaver_resolved_schema/Cargo.toml Adds serde_yaml dependency for new YAML-value loading path.
crates/weaver_forge/src/v2/registry.rs Updates tests to use parsed FileFormat for resolved schemas.
crates/weaver_common/src/lib.rs Adds common error variants for file-format validation and exports file_format module.
crates/weaver_common/src/file_format.rs New shared FileFormat type with parsing, validation, and JSON schema representation.
CHANGELOG.md Documents forward-compatible handling of unknown fields for newer-minor formats.
Cargo.lock Locks new serde_yaml dependency usage.
Comments suppressed due to low confidence (1)

crates/weaver_semconv/src/manifest.rs:184

  • When file_format is present but has an incompatible prefix/major, the code currently validates it only after requiring publication-only fields like schema_url and resolved_registry_uri. This can mask a major/prefix mismatch behind a "missing required field" error, which is less actionable and can contradict the stated behavior that major mismatches fail immediately.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/weaver_semconv/src/manifest.rs
Comment thread crates/weaver_semconv/src/manifest.rs Outdated
Comment thread crates/weaver_common/src/file_format.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 42 out of 43 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

crates/weaver_semconv/src/manifest.rs:294

  • This comment still refers to file_format being deserialized as a String and uses numeric→String coercion as an example, but RawManifestFields.file_format is now a FileFormat. Updating the example avoids confusion about why the file is deserialized twice.
    crates/weaver_semconv/src/manifest.rs:184
  • If file_format is present but has the wrong prefix/major, and other required publication fields are missing, this branch currently errors on the missing field first. That can hide the more actionable file_format incompatibility. Consider validating prefix/major immediately when file_format is present so the user sees the file-format error even if schema_url/resolved_registry_uri are also invalid.
    crates/weaver_common/src/file_format.rs:142
  • parse_file_format_version claims it rejects strings that don't match the prefix/MAJOR.MINOR pattern, but it currently accepts an empty prefix (e.g. "/2.0"). Rejecting an empty prefix makes the parser match the documented format and the JSON-schema pattern more closely.
/// Parses a `"type/MAJOR.MINOR"` file-format string.
///
/// Returns `Some((prefix, major, minor))` or `None` if the string doesn't match the pattern.
#[must_use]
pub fn parse_file_format_version(s: &str) -> Option<(&str, u32, u32)> {
    let (prefix, ver) = s.split_once('/')?;
    let (major_s, minor_s) = ver.split_once('.')?;
    Some((prefix, major_s.parse().ok()?, minor_s.parse().ok()?))
}

Comment thread crates/weaver_semconv/src/lib.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants