Attribute groups in schema v2 by lmolkova · Pull Request #933 · open-telemetry/weaver

lmolkova · 2025-09-12T06:10:47Z

TODO:

changelog
tests
JSON schema and doc

Semconv PR to update http and DB: open-telemetry/semantic-conventions@main...lmolkova:semantic-conventions:schema-v2-http-db

Example

  - id: attributes.db.client.authority
    visibility: internal
    attributes:
      - ref: server.address
        brief: Name of the database host.
      - ref: server.port
        requirement_level:
          conditionally_required: If using a port other than the default port for this DBMS and if `server.address` is set.

metrics:
  - name: db.client.operation.duration
    ...
    attributes:
      - ref_group: attributes.db.client.query
      - ref_group: attributes.db.client.response
      - ref_group: attributes.db.client.authority
      - ref_group: attributes.db.client.network
      - ref_group: attributes.db.client.operation
      - ref: db.system.name
        requirement_level: required
      - ref: db.stored_procedure.name
        requirement_level:
          recommended: If operation applies to a specific stored procedure.
...

github-advanced-security

clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

crates/weaver_semconv/src/v2/attribute_group.rs

crates/weaver_resolver/src/registry.rs

crates/weaver_resolved_schema/src/registry.rs

crates/weaver_resolver/src/registry.rs

crates/weaver_semconv/src/v2/attribute_group.rs

crates/weaver_resolver/data/registry-test-v2-2-multifile/expected-registry.json

codecov · 2025-09-22T22:52:52Z

Codecov Report

❌ Patch coverage is 94.48819% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.3%. Comparing base (8e36299) to head (820fcc6).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/weaver_semconv/src/v2/attribute_group.rs	84.0%	4 Missing ⚠️
crates/weaver_resolved_schema/src/registry.rs	0.0%	1 Missing ⚠️
crates/weaver_resolver/src/registry.rs	98.2%	1 Missing ⚠️
crates/weaver_semconv/src/v2/mod.rs	93.3%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #933     +/-   ##
=======================================
+ Coverage   78.0%   78.3%   +0.3%     
=======================================
  Files         76      77      +1     
  Lines       6021    6121    +100     
=======================================
+ Hits        4699    4796     +97     
- Misses      1322    1325      +3

☔ 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.

crates/weaver_semconv/src/v2/attribute_group.rs

.github/workflows/ci.yml

…attribute-groups-v2

thompson-tomo

I also might have missed it but are we emitting public attribute groups as a part of the resolved schema and can they be distinguished from internal?

thompson-tomo · 2025-09-26T09:49:34Z

schemas/semconv-syntax.v2.md

+
+A public attribute group definition consists of the following properties:
+
+- `id` - Required. Uniquely identifies the attribute group.


Should we not call this key and then generate an id for the group?

This key should also be included in the resolved schema as a property of the attribute on a signal when it is a public group otherwise empty. This is to help with documentation.

I don't understand the proposal. This is a group id, key is used for attributes themselves, id matches today's structure and would be more intuitive than key.

Public group ids would eventually be available on via lineage in the resolved schema after we figure out how lineage should work in v2 (#933 (comment))

I see it as the same as attributes as you define a key which you then use to reference it from within an attributes array. Whereas an id should be unique across signals which is why id is usually prefixed with the signal type.

If it is more intuitive then it should be the same for attributes given they aim for the same thing which is adding 1 or more attributes to a signal.

I agree lineage could be made to work but it currently doesn't work. we would need to ensure that it can be distinguished from simply coming from the registry. This is so we can link from a span to the public group definition which can provide additional details about sourcing the values for instance.

I don't agree that attributes are the same as attribute groups. Also I don't see how renaming id to key makes anything better.

For me It is all about how it would be used.

Let's think about some of the use cases where an identifying label for a group could/would be used:

rendering a title of the group in docs. Currently string manipulation is done

Rendered in attribute tables on signals so link to group can be implemented.

Added on signal definitions to add the attributes

Code generating methods to add the groups to signals

Hence considering the above and that id's are implicitly used for internal linking, we should either add a name alongside the id with both being user defined or internally generate the id based on the key and type.

lmolkova · 2025-09-26T15:43:57Z

I also might have missed it but are we emitting public attribute groups as a part of the resolved schema and can they be distinguished from internal?

yes, please take a look at the syntax doc.

lquerel

LGTM

thompson-tomo · 2025-09-27T01:14:56Z

I also might have missed it but are we emitting public attribute groups as a part of the resolved schema and can they be distinguished from internal?
yes, please take a look at the syntax doc.

Isn't the syntax document describing what a user writes (un-resolved), whereas the resolved is what is emitted by Weaver? In effect shouldn't we have the visibility field included in https://github.com/open-telemetry/weaver/blob/561ff28a4e3e81f9175457cda7bf63199b59fd5b/crates/weaver_resolved_schema/src/registry.rs

Also doesn't your comment in #933 (comment) confirm that groups are no longer present?

lmolkova · 2025-09-27T04:33:32Z

I also might have missed it but are we emitting public attribute groups as a part of the resolved schema and can they be distinguished from internal?
yes, please take a look at the syntax doc.

Isn't the syntax document describing what a user writes (un-resolved), whereas the resolved is what is emitted by Weaver? In effect shouldn't we have the visibility field included in https://github.com/open-telemetry/weaver/blob/561ff28a4e3e81f9175457cda7bf63199b59fd5b/crates/weaver_resolved_schema/src/registry.rs

Also doesn't your comment in #933 (comment) confirm that groups are no longer present?

this PR does not implement resolved schema v2 - it does not exist yet, @jsuereth is working on it. After this PR v2 internal groups are do not appear in the resolve schema. public groups do. All v1 groups appear. It's still an intermediate state

github-advanced-security bot found potential problems Sep 12, 2025

View reviewed changes

lmolkova force-pushed the attribute-groups-v2 branch 2 times, most recently from 752c9f1 to 2db3a6e Compare September 12, 2025 15:45

jsuereth reviewed Sep 18, 2025

View reviewed changes

crates/weaver_semconv/src/v2/attribute_group.rs Outdated Show resolved Hide resolved

jsuereth reviewed Sep 18, 2025

View reviewed changes

crates/weaver_resolver/src/registry.rs Outdated Show resolved Hide resolved

jsuereth reviewed Sep 18, 2025

View reviewed changes

crates/weaver_resolved_schema/src/registry.rs Outdated Show resolved Hide resolved

jsuereth reviewed Sep 18, 2025

View reviewed changes

crates/weaver_resolver/src/registry.rs Show resolved Hide resolved

lmolkova added 3 commits September 22, 2025 11:45

attribute groups v2

d9a1466

refactor and clean up registry

7996491

review: cleanup, more tests, docs

29bd639

lmolkova force-pushed the attribute-groups-v2 branch from 2db3a6e to 29bd639 Compare September 22, 2025 22:46

github-advanced-security bot found potential problems Sep 22, 2025

View reviewed changes

lmolkova commented Sep 22, 2025

View reviewed changes

crates/weaver_resolver/data/registry-test-v2-2-multifile/expected-registry.json Show resolved Hide resolved

also suppress clippy on unused_qualifications

cb26fac

github-advanced-security bot found potential problems Sep 22, 2025

View reviewed changes

crates/weaver_semconv/src/v2/attribute_group.rs Fixed Show fixed Hide fixed

crates/weaver_semconv/src/v2/attribute_group.rs Fixed Show fixed Hide fixed

crates/weaver_semconv/src/v2/attribute_group.rs Fixed Show fixed Hide fixed

lmolkova added 2 commits September 22, 2025 16:09

more clippy suppressions and proofread doc

b307ff6

changelog

b668231

lmolkova marked this pull request as ready for review September 22, 2025 23:15

lmolkova requested a review from a team as a code owner September 22, 2025 23:15

lquerel requested changes Sep 23, 2025

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

lmolkova added 3 commits September 25, 2025 19:13

Merge branch 'main' of https://github.com/open-telemetry/weaver into …

ddc0493

…attribute-groups-v2

fix unused qualifications

eac4eca

clean up

561ff28

thompson-tomo reviewed Sep 26, 2025

View reviewed changes

lquerel approved these changes Sep 26, 2025

View reviewed changes

jsuereth approved these changes Oct 8, 2025

View reviewed changes

Merge branch 'main' into attribute-groups-v2

820fcc6

jerbly merged commit 8462991 into open-telemetry:main Oct 8, 2025
24 checks passed


		A public attribute group definition consists of the following properties:

		- `id` - Required. Uniquely identifies the attribute group.

Conversation

lmolkova commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thompson-tomo left a comment

Choose a reason for hiding this comment

Uh oh!

thompson-tomo Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

lmolkova Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

thompson-tomo Sep 27, 2025 • 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 Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

thompson-tomo Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

lmolkova commented Sep 26, 2025

Uh oh!

lquerel left a comment

Choose a reason for hiding this comment

Uh oh!

thompson-tomo commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmolkova commented Sep 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lmolkova commented Sep 12, 2025 •

edited

Loading

codecov bot commented Sep 22, 2025 •

edited

Loading

thompson-tomo Sep 27, 2025 •

edited

Loading

thompson-tomo commented Sep 27, 2025 •

edited

Loading