Skip to content

Fail on unstructured deprecated property under --future flag#737

Merged
lquerel merged 7 commits intoopen-telemetry:mainfrom
lmolkova:deprecated-warn-on-unstructured-deprecated
May 27, 2025
Merged

Fail on unstructured deprecated property under --future flag#737
lquerel merged 7 commits intoopen-telemetry:mainfrom
lmolkova:deprecated-warn-on-unstructured-deprecated

Conversation

@lmolkova
Copy link
Copy Markdown
Member

we don't have means to enforce using structured deprecation in semconv since deprecated defaults to Uncategorized during deserialization and it's not possible to distinguish deprecated: note from deprecated: \n reason: unstructured \n note: note.

Fixing it.

@lmolkova lmolkova requested a review from a team as a code owner May 11, 2025 18:05
@lmolkova lmolkova requested a review from Copilot May 11, 2025 18:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 enforces structured deprecation in semantic conventions by failing on unstructured deprecated properties under the --future flag. Key changes include updating expected test violation counts, introducing a new error variant for unstructured deprecated properties, and modifying deprecation parsing and messaging across modules.

Reviewed Changes

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

Show a summary per file
File Description
tests/registry_generate.rs Updated expected policy violation count to account for the new error.
tests/registry_check.rs Adjusted expectation and inline comment to reflect one additional violation.
src/registry/check.rs Incorporated the new unstructured deprecated property error into validation logic.
crates/weaver_semconv/src/lib.rs Added a new Error variant (UnstructuredDeprecatedProperty) with updated error messaging.
crates/weaver_semconv/src/group.rs Inserted new checks for unstructured deprecated notes on groups and attributes.
crates/weaver_semconv/src/deprecated.rs Modified deprecation deserialization to use Deprecated::Unspecified for string inputs.
crates/weaver_resolved_schema/src/lib.rs Updated pattern matching to accommodate the revised deprecation variants.
crates/weaver_live_check/src/advice.rs Adjusted handling of deprecation variants to reflect changes in naming.
crates/weaver_forge/src/extensions/otel.rs Revised output message for deprecated properties to align with updated naming.
crates/weaver_forge/expected_output/test/resource/library.md Updated deprecation output in documentation.
crates/weaver_forge/data/exporter.yaml Modified deprecation fields from legacy string format to structured mapping.
crates/weaver_codegen_test/semconv_registry/registry/deprecated/network.yaml Aligned deprecated field formatting across network attributes.
Comments suppressed due to low confidence (2)

crates/weaver_semconv/src/deprecated.rs:80

  • [nitpick] Consider standardizing the deprecation variant naming between the string branch (using 'Unspecified') and the map branch (using 'Uncategorized') to reduce potential confusion and improve maintainability.
Ok(Deprecated::Unspecified { note: value.to_owned() })

crates/weaver_forge/src/extensions/otel.rs:852

  • [nitpick] Verify that changing the output label to 'unspecified' is intentional and that it aligns consistently with the updated deprecation error messaging across all modules.
"unspecified: Replaced by new_name."

Comment thread tests/registry_check.rs
@codecov
Copy link
Copy Markdown

codecov Bot commented May 11, 2025

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 76.7%. Comparing base (04f911e) to head (4e31c0d).

Files with missing lines Patch % Lines
crates/weaver_resolved_schema/src/lib.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #737   +/-   ##
=====================================
  Coverage   76.7%   76.7%           
=====================================
  Files         65      65           
  Lines       5036    5046   +10     
=====================================
+ Hits        3864    3873    +9     
- Misses      1172    1173    +1     

☔ View full report in Codecov by Sentry.
📢 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.

Comment thread Cargo.lock
Comment thread crates/weaver_semconv/src/deprecated.rs Outdated
Comment thread crates/weaver_semconv/src/deprecated.rs Fixed
@lmolkova lmolkova force-pushed the deprecated-warn-on-unstructured-deprecated branch 2 times, most recently from 5ec290b to 5154f1d Compare May 16, 2025 20:33
@lmolkova
Copy link
Copy Markdown
Member Author

@jsuereth @lquerel @jerbly could you please take another look?

@lquerel
Copy link
Copy Markdown
Contributor

lquerel commented May 27, 2025

@lmolkova Today I'm preparing a release, could you:

  • fix the conflict in your PR so I can include it into the next release,
  • update the CHANGELOG.md

Thanks

@lmolkova lmolkova force-pushed the deprecated-warn-on-unstructured-deprecated branch from 4e31c0d to 0135bcd Compare May 27, 2025 17:40
@lquerel lquerel enabled auto-merge (squash) May 27, 2025 17:55
@lquerel lquerel merged commit f60d687 into open-telemetry:main May 27, 2025
33 of 34 checks passed
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.

5 participants