Fix resolution for dependencies by lmolkova · Pull Request #1280 · open-telemetry/weaver

lmolkova · 2026-03-11T21:47:55Z

Both v1 and v2 resolution has a problem when using dependencies - it returns random attribute from attribute catalog instead of definition resulting in wrong briefs/notes, etc.

E.g. in this yaml https://github.com/lmolkova/semconv-scale23x-demo/blob/965f9f0a6094e1d21150d4a0441ede7aafbb4a0b/conventions/src/registry.yaml#L36 ( - ref: server.address from semconv), the server.address resolves to "Name of the database host." which comes from DB refinement.

https://github.com/lmolkova/semconv-scale23x-demo/blob/2d31c5bf2437f5936d8740a338845bcb06ced456/docs/storage/metrics.md#attributes-1

This PR fixes those. It does not solve multi-dependency/from-transitive-dependency resolution.

lmolkova · 2026-03-11T21:49:45Z

crates/weaver_resolved_schema/src/catalog.rs

+    /// Attribute definitions available in this registry (including those
+    /// from dependencies). Used for cross-registry attribute lookup.
+    /// Not serialized — populated only for freshly resolved schemas.
+    root_attributes: HashMap<String, (Attribute, String)>,


this is the real change for v1, let's populate root_attributes directly instead of trying to use lineage to infer if something in the catalog is original definition.

The rest in this file is cleanup - this struct should not allow to mutate attrs, does not need to be (de)serializable, many of its features were only used in tests

lmolkova · 2026-03-11T21:50:59Z

crates/weaver_resolver/data/multi-registry/otel_registry/otel_registry.yaml

        stability: stable
        requirement_level: recommended
        examples: "Ex"
+  - id: db.client.metrics


this is repro for the v1 issue

Ah - I gotcha, we don't use extends here, so all the assumptions fail, and the catalog is ambiguous. I thought it'd be safe to remove the hardcoded registry. paths for V1, but looks like not, thanks for catching this.

lmolkova · 2026-03-11T21:51:41Z

crates/weaver_resolver/data/registry-test-1-single-attr-ref/expected-attribute-catalog.json

-      "requirement_level": "recommended",
-      "note": "Destination name SHOULD uniquely identify a specific queue, topic or other entity within the broker. If\nthe broker doesn't have such notion, the destination name SHOULD uniquely identify the broker.\n",
-      "stability": "stable"
+[


no change here except unwrapping list of attrs - this allows to remove serialization from Catalog

OOF, yeah you can blame me for that change and I regret doing it.

I wonder if maybe serializing this to YAML or TOML or something would make this more readable / easier to diff in the future. WDYT?

lmolkova · 2026-03-11T21:53:26Z

crates/weaver_resolver/data/registry-test-v2-dep/app_registry/manifest.yaml

+    # TODO: this only works with definition registry, but not with
+    # resolved one, because resolved does not know how to get
+    # attributes from transitive dependencies.
+    registry_path: data/registry-test-v2-dep/consumer_registry


fun around attrs from transitive dependencies:

if you use definition as a dependency, you can use attributes in transitive one

if you use resolved as dependency, you can't - they are not there

Let's make it consistent (regardless of the approach we take)

also more fun: since only one dependency is allowed, you can't really make

A -> resolved B -> C work today, unless you can modify B to import things

lmolkova · 2026-03-11T21:56:22Z

crates/weaver_resolver/src/attribute.rs

-                })
-        });
-        result
+        self.catalog


this is part of solution for v1 - use root attributes instead of refs, the logic here didn't work for dependencies

lmolkova · 2026-03-11T21:57:40Z

crates/weaver_resolver/src/attribute.rs

        let fake_group_id = format!("v2_dependency.{}", self.schema_url.name());
-        self.attribute_catalog.iter().find_map(|attr| {
+        self.registry.attributes.iter().find_map(|attr_ref| {
+            let attr = self.attribute_catalog.get(attr_ref.0 as usize)?;


this is solution for v2 - attribute_catalog contains definitions and refinements. given lookup by key, we can only return the original definition from registry.attributes

Doh! Yeah - I can't believe I missed this the first time.

codecov · 2026-03-11T22:01:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.1%. Comparing base (7678d00) to head (93b2230).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1280     +/-   ##
=======================================
+ Coverage   81.0%   81.1%   +0.1%     
=======================================
  Files        111     111             
  Lines       9380    9375      -5     
=======================================
+ Hits        7598    7606      +8     
+ Misses      1782    1769     -13

☔ 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.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jsuereth · 2026-03-11T22:03:21Z

crates/weaver_resolved_schema/src/lib.rs

    /// and signals.
    pub catalog: Catalog,
    /// Resource definition (only for application).
-    #[serde(skip_serializing_if = "Option::is_none")]


Is this intentional?

yep, it does not need json schema - we don't (and don't need to) support it for v1. And it's not (de)serializable either

I'm trying to remove serialization from things that are not intended to be read or written by weaver. This helps to understand what's internal detail and what's a real contract

lmolkova added 5 commits March 11, 2026 12:59

Fix resolution from dependencies

b016a95

clean up

7cbd905

more clean up in catalog

efee834

more cleaup

d2b9f6b

more cleaup

10a317c

lmolkova requested a review from a team as a code owner March 11, 2026 21:47

lmolkova commented Mar 11, 2026

View reviewed changes

jsuereth reviewed Mar 11, 2026

View reviewed changes

exclude json from typos

93b2230

jsuereth approved these changes Mar 11, 2026

View reviewed changes

jsuereth merged commit ff72026 into open-telemetry:main Mar 11, 2026
26 checks passed

Conversation

lmolkova commented Mar 11, 2026

Uh oh!

lmolkova Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmolkova Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmolkova Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lmolkova Mar 11, 2026 •

edited

Loading

lmolkova Mar 11, 2026 •

edited

Loading

codecov bot commented Mar 11, 2026 •

edited

Loading

lmolkova Mar 11, 2026 •

edited

Loading