Skip to content

Commit 9718582

Browse files
committed
fix reg_group resolution
1 parent a177fb6 commit 9718582

5 files changed

Lines changed: 178 additions & 29 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ All notable changes to this project will be documented in this file.
44

55
# Unreleased
66

7+
- Fix v2 attribute resolution so a `ref` inside an included group (`ref_group`) merges field-by-field instead of replacing the whole attribute. ([#TODO](https://github.com/open-telemetry/weaver/pull/TODO) by @lmolkova)
8+
79
# [0.25.0] - 2026-07-24
810

911
- Use semantic conventions v2 for `weaver registry infer`. ([#1334](https://github.com/open-telemetry/weaver/pull/1334) by @ArthurSens)

crates/weaver_resolver/data/registry-test-v2-4-refinements/expected-attribute-catalog.json

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,37 @@
2222
"stability": "stable",
2323
"role": "identifying"
2424
},
25+
{
26+
"name": "test.attr",
27+
"type": "int",
28+
"brief": "Test attribute",
29+
"requirement_level": {
30+
"conditionally_required": "on the base signal"
31+
},
32+
"stability": "stable"
33+
},
34+
{
35+
"name": "test.attr",
36+
"type": "int",
37+
"brief": "Test attribute",
38+
"examples": [
39+
42
40+
],
41+
"requirement_level": "recommended",
42+
"stability": "stable"
43+
},
44+
{
45+
"name": "test.attr",
46+
"type": "int",
47+
"brief": "Test attribute",
48+
"examples": [
49+
42
50+
],
51+
"requirement_level": {
52+
"conditionally_required": "on the base signal"
53+
},
54+
"stability": "stable"
55+
},
2556
{
2657
"name": "test.attr2",
2758
"type": "string",

crates/weaver_resolver/data/registry-test-v2-4-refinements/expected-registry.json

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
"stability": "stable",
99
"attributes": [
1010
2,
11-
5
11+
8
1212
],
1313
"name": "base.entity",
1414
"lineage": {
@@ -51,8 +51,8 @@
5151
"stability": "stable",
5252
"attributes": [
5353
0,
54-
3,
55-
6
54+
6,
55+
9
5656
],
5757
"name": "entity.refined.entity",
5858
"lineage": {
@@ -134,7 +134,7 @@
134134
"brief": "Base metric",
135135
"stability": "stable",
136136
"attributes": [
137-
1
137+
3
138138
],
139139
"metric_name": "base.metric",
140140
"instrument": "histogram",
@@ -150,8 +150,10 @@
150150
"inherited_fields": [
151151
"brief",
152152
"note",
153-
"requirement_level",
154153
"stability"
154+
],
155+
"locally_overridden_fields": [
156+
"requirement_level"
155157
]
156158
}
157159
},
@@ -176,8 +178,8 @@
176178
"brief": "Refined metric",
177179
"stability": "stable",
178180
"attributes": [
179-
1,
180-
7
181+
5,
182+
10
181183
],
182184
"metric_name": "base.metric",
183185
"instrument": "histogram",
@@ -194,8 +196,11 @@
194196
"inherited_fields": [
195197
"brief",
196198
"note",
197-
"requirement_level",
198199
"stability"
200+
],
201+
"locally_overridden_fields": [
202+
"examples",
203+
"requirement_level"
199204
]
200205
},
201206
"test.attr3": {
@@ -233,8 +238,8 @@
233238
"brief": "<synthetic v2>",
234239
"attributes": [
235240
1,
236-
4,
237-
7
241+
7,
242+
10
238243
],
239244
"lineage": {
240245
"provenance": {
@@ -278,7 +283,7 @@
278283
"brief": "Test group",
279284
"stability": "stable",
280285
"attributes": [
281-
1
286+
3
282287
],
283288
"lineage": {
284289
"provenance": {
@@ -291,8 +296,10 @@
291296
"inherited_fields": [
292297
"brief",
293298
"note",
294-
"requirement_level",
295299
"stability"
300+
],
301+
"locally_overridden_fields": [
302+
"requirement_level"
296303
]
297304
}
298305
}

crates/weaver_resolver/data/registry-test-v2-4-refinements/registry/v2-refinements.yaml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,19 @@ attribute_groups:
1919
brief: Test group
2020
stability: stable
2121
attributes:
22+
# Base sets a non-default requirement level on test.attr.
2223
- ref: test.attr
24+
requirement_level:
25+
conditionally_required: "on the base signal"
2326
- id: test.refine.extras
2427
visibility: internal
2528
attributes:
29+
# Re-refs test.attr with ONLY an example (no requirement_level). When this
30+
# group is pulled into a refinement via `ref_group`, it must NOT reset the
31+
# `conditionally_required` inherited from test.group above back to the
32+
# `recommended` default.
33+
- ref: test.attr
34+
examples: [42]
2635
- ref: test.attr3
2736
requirement_level: recommended
2837

crates/weaver_resolver/src/registry.rs

Lines changed: 117 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -867,26 +867,45 @@ fn resolve_inheritance_attrs_unified(
867867
// Note: we use a BTreeMap to ensure that the attributes are sorted by
868868
// their id in the resolved registry. This is useful for unit tests to
869869
// ensure that the resolved registry is easy to compare.
870-
let mut inherited_attrs = BTreeMap::new();
871-
872-
// Inherit the attributes from all included groups.
870+
let mut inherited_attrs: BTreeMap<String, AttrWithLineage> = BTreeMap::new();
871+
872+
// Inherit the attributes from all bases (parent signal + included groups),
873+
// lowest priority first. When the same attribute id appears in more than one
874+
// base, the higher-priority base overrides the lower one *field by field*:
875+
// fields it leaves unset inherit from the lower-priority base instead of
876+
// replacing the whole spec. This mirrors how inline `ref:` overrides are
877+
// merged below, so a bare `ref:` inside a `ref_group` behaves the same as a
878+
// bare inline `ref:`.
873879
for (parent_group_id, included_group) in include_groups {
874880
for parent_attr in included_group.iter() {
875881
let attr_id = parent_attr.spec.id();
876-
let lineage = AttributeLineage::inherit_from(parent_group_id, &parent_attr.spec);
877-
log::debug!(
878-
"Inheriting attribute {} from group {}, resolved to {:#?}",
879-
attr_id,
880-
parent_group_id,
881-
lineage.source_group
882-
);
883-
_ = inherited_attrs.insert(
884-
attr_id.clone(),
885-
AttrWithLineage {
886-
spec: parent_attr.spec.clone(),
887-
lineage,
888-
},
889-
);
882+
match inherited_attrs.get_mut(&attr_id) {
883+
Some(existing) => {
884+
existing.spec = resolve_inheritance_attr(
885+
&parent_attr.spec,
886+
&existing.spec,
887+
&mut existing.lineage,
888+
);
889+
existing.lineage.source_group = parent_group_id.to_owned();
890+
}
891+
None => {
892+
let lineage =
893+
AttributeLineage::inherit_from(parent_group_id, &parent_attr.spec);
894+
log::debug!(
895+
"Inheriting attribute {} from group {}, resolved to {:#?}",
896+
attr_id,
897+
parent_group_id,
898+
lineage.source_group
899+
);
900+
_ = inherited_attrs.insert(
901+
attr_id.clone(),
902+
AttrWithLineage {
903+
spec: parent_attr.spec.clone(),
904+
lineage,
905+
},
906+
);
907+
}
908+
}
890909
}
891910
}
892911

@@ -1167,8 +1186,10 @@ mod tests {
11671186

11681187
use crate::attribute::AttributeCatalog;
11691188
use crate::registry::cleanup_and_stabilize_catalog_and_registry;
1189+
use crate::registry::resolve_inheritance_attrs_unified;
11701190
use crate::registry::UnresolvedGroup;
11711191
use crate::registry::UnresolvedRegistry;
1192+
use weaver_resolved_schema::attribute::UnresolvedAttribute;
11721193
use crate::{WeaverResolver, WeaverResolverConfig};
11731194
use std::sync::Arc;
11741195

@@ -1854,4 +1875,83 @@ groups:
18541875
fn to_json<T: Serialize + ?Sized>(value: &T) -> String {
18551876
serde_json::to_string_pretty(value).unwrap()
18561877
}
1878+
1879+
fn bare_ref(
1880+
id: &str,
1881+
requirement_level: Option<weaver_semconv::attribute::RequirementLevel>,
1882+
examples: Option<weaver_semconv::attribute::Examples>,
1883+
) -> UnresolvedAttribute {
1884+
UnresolvedAttribute {
1885+
spec: weaver_semconv::attribute::AttributeSpec::Ref {
1886+
r#ref: id.to_owned(),
1887+
brief: None,
1888+
examples,
1889+
tag: None,
1890+
requirement_level,
1891+
sampling_relevant: None,
1892+
note: None,
1893+
stability: None,
1894+
deprecated: None,
1895+
prefix: false,
1896+
annotations: None,
1897+
role: None,
1898+
},
1899+
}
1900+
}
1901+
1902+
/// A bare `ref` inside an included group (`ref_group`) that only narrows the
1903+
/// example must NOT reset the `requirement_level` inherited from a
1904+
/// lower-priority base (`extends`). It must behave the same as a bare inline
1905+
/// `ref`: unset fields inherit rather than replacing the whole spec.
1906+
///
1907+
/// Regression for the messaging `destination.name` flip
1908+
/// (`conditionally_required` -> `recommended`) caused by resolving-then-
1909+
/// merging `ref_group` bases instead of merging their fields in place.
1910+
#[test]
1911+
fn ref_group_bare_ref_preserves_inherited_requirement_level() {
1912+
use weaver_semconv::attribute::{AttributeSpec, Examples, RequirementLevel};
1913+
1914+
let cond = RequirementLevel::ConditionallyRequired {
1915+
text: "If span describes operation on a single message.".to_owned(),
1916+
};
1917+
1918+
// Lowest priority base (parent `extends`): sets conditionally_required.
1919+
let parent = vec![bare_ref("messaging.destination.name", Some(cond.clone()), None)];
1920+
// Higher priority base (`ref_group`): only sets an example, no level.
1921+
let included = vec![bare_ref(
1922+
"messaging.destination.name",
1923+
None,
1924+
Some(Examples::String("MyTopic".to_owned())),
1925+
)];
1926+
1927+
let resolved = resolve_inheritance_attrs_unified(
1928+
"span.messaging.rocketmq.send.producer",
1929+
&[], // no inline `ref:` overrides on the refinement itself
1930+
vec![
1931+
("messaging.attributes.common", parent.as_slice()),
1932+
("messaging.rocketmq.attributes.common", included.as_slice()),
1933+
],
1934+
None,
1935+
);
1936+
1937+
assert_eq!(resolved.len(), 1);
1938+
match &resolved[0].spec {
1939+
AttributeSpec::Ref {
1940+
requirement_level,
1941+
examples,
1942+
..
1943+
} => {
1944+
assert_eq!(
1945+
requirement_level,
1946+
&Some(cond),
1947+
"ref_group must not reset the inherited requirement_level"
1948+
);
1949+
assert!(
1950+
matches!(examples, Some(Examples::String(s)) if s == "MyTopic"),
1951+
"ref_group example override should still win"
1952+
);
1953+
}
1954+
other => panic!("expected a Ref attribute, got {other:?}"),
1955+
}
1956+
}
18571957
}

0 commit comments

Comments
 (0)