Skip to content

Use name or metric_name as namespace for weaver diff on signal differences.#784

Merged
jsuereth merged 9 commits intoopen-telemetry:mainfrom
jsuereth:wip-metric-diff
Jun 12, 2025
Merged

Use name or metric_name as namespace for weaver diff on signal differences.#784
jsuereth merged 9 commits intoopen-telemetry:mainfrom
jsuereth:wip-metric-diff

Conversation

@jsuereth
Copy link
Copy Markdown
Contributor

@jsuereth jsuereth commented Jun 11, 2025

Fixes #776. First step for #785.

This PR:

  • Updates diff algorithm to pull groups by name instead of id (except metric groups, which use metric_name.
    • Today in weaver, name is guaranteed to be unique via validation done on resolved registries. So is metric_name.
    • extends is wonky on signals, as they cannot actually use the same name, so that use case isn't supported.
  • Also fixes some missing Resource => Entity renames I noticed.

Out of Scope for this PR:

  • Fixing the "attribute registry" hackery for weaver diff. I have a simple path forward laid out in Path forward for id vs name and weaver diff #785 that I think can be a quick follow up
  • Providing actual "refinement" groups that I think we'll need for multi-registry.

Notes:

  • weaver diff integration tests actually already used name on span groups.

Comment thread crates/weaver_resolved_schema/src/lib.rs Fixed
Comment thread crates/weaver_resolved_schema/src/registry.rs Fixed
Comment thread crates/weaver_resolved_schema/src/registry.rs Fixed
Comment thread crates/weaver_resolved_schema/src/lib.rs Fixed
…renamed to. However, we need a general discussion on what these signal changes mean overall.
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 11, 2025

Codecov Report

Attention: Patch coverage is 75.60976% with 10 lines in your changes missing coverage. Please review.

Project coverage is 76.3%. Comparing base (dbf27e3) to head (360a775).

Files with missing lines Patch % Lines
crates/weaver_resolved_schema/src/registry.rs 30.0% 7 Missing ⚠️
crates/weaver_resolved_schema/src/lib.rs 90.3% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #784     +/-   ##
=======================================
+ Coverage   75.9%   76.3%   +0.3%     
=======================================
  Files         69      69             
  Lines       5510    5545     +35     
=======================================
+ Hits        4187    4233     +46     
+ Misses      1323    1312     -11     

☔ 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 crates/weaver_resolved_schema/src/lib.rs Fixed
Comment thread crates/weaver_resolved_schema/src/lib.rs Fixed
Comment thread crates/weaver_resolved_schema/src/registry.rs Fixed
@jsuereth jsuereth marked this pull request as ready for review June 11, 2025 23:46
@jsuereth jsuereth requested a review from a team as a code owner June 11, 2025 23:46
Copy link
Copy Markdown
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

LGTM.

@jsuereth jsuereth changed the title [DO NOT MERGE] WIP Fix for #776. Fix for #776. Jun 12, 2025
@jsuereth jsuereth moved this to Next Release in OTel Weaver Project Jun 12, 2025
@jsuereth jsuereth changed the title Fix for #776. Use name or metric_name as namespace for weaver diff on signal differences. Jun 12, 2025
@jsuereth jsuereth merged commit 66183e0 into open-telemetry:main Jun 12, 2025
23 checks passed
@github-project-automation github-project-automation Bot moved this from Next Release to Done in OTel Weaver Project Jun 12, 2025
@jsuereth jsuereth deleted the wip-metric-diff branch January 26, 2026 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

weaver registry diff: old_name is a group id and not a metric/event name

3 participants