feat: Add support for event body fields #297

MSNev · 2024-08-07T16:37:37Z

Adds support for the event body fields reusing the existing event type.

Supports existing event (Span) definition
Supports the log based event definition as an extension of the existing span definition
Adds Body (optional) definition which supports a collection of fields or a single string value
- Body has a type of string or map
- An optional brief (required for string type)
- An optional note
- An optional stability
- An optional examples (required for string type)

MSNev · 2024-08-07T16:38:27Z

The PR supersedes #236 as discussed in tools sigs this morning.

crates/weaver_resolver/data/registry-test-4-events/registry/browser-event.yaml

defaults/jq/semconv.jq

crates/weaver_semconv/src/group.rs

crates/weaver_resolver/data/registry-test-4-events/registry/browser-event.yaml

crates/weaver_resolver/data/registry-test-4-events/registry/browser-ref-event.yaml

crates/weaver_resolver/data/registry-test-4-events/registry/browser-event.yaml

crates/weaver_resolver/data/registry-test-4-events/registry/browser-ref-event.yaml

crates/weaver_semconv/src/group.rs

defaults/jq/semconv.jq

lquerel

This PR represents a lot of work and is overall really good. However, my main concern is the lack of alignment between the concept of a body field and the concept of embedded/aliased attributes. Depending on how this discussion ends, the implications for this PR could be more or less significant.

crates/weaver_forge/src/registry.rs

crates/weaver_resolved_schema/src/body.rs

crates/weaver_resolver/data/registry-test-4-events/registry/browser-ref-event.yaml

crates/weaver_resolved_schema/src/body.rs

crates/weaver_resolver/src/attribute.rs

crates/weaver_semconv/src/group.rs

crates/weaver_semconv/src/body.rs

crates/weaver_resolver/src/body.rs

crates/weaver_semconv/src/body.rs

crates/weaver_resolved_schema/src/body.rs

crates/weaver_semconv/src/body.rs

MSNev · 2024-09-17T01:54:46Z

Please also see comments and discussion topics in #365

MSNev · 2024-09-18T23:11:13Z

AnyValue PR and this PR now have the same (common) set of changes.

- type - brief - note - stability - examples

jsuereth

This looks good to me now.

I'm still unhappy with the direction our tests have been going in and I'd much prefer finer-grained, feature or regresison focused tests, but we can clean that up later.

crates/weaver_semconv/src/group.rs

crates/weaver_resolver/data/registry-test-4-events/registry/trace-gen-ai.yaml

crates/weaver_resolver/data/registry-test-4-events/expected-registry.json

lquerel

@MSNev, @jsuereth I haven’t finished the review, but I will do it in a few hours after my meetings. In the meantime, I’m posting some of my preliminary comments. This is definitely heading in the right direction!

crates/weaver_resolver/data/registry-test-4-events/observed-events.json

crates/weaver_resolver/data/registry-test-4-events/registry/trace-gen-ai.yaml

lquerel · 2024-09-20T17:45:12Z

crates/weaver_forge/templates/test/event.md


 {% for event in ctx.events %}
-## Event `{{ event.name }}`
+## Event `{{ event.id }}`


Just a question: Why using id and not name here?
Not blocking.

Actually, the top level "name" will still exist on the event type, its just the AnyValues that will now only have the id (and no name), and this is just local test code.

lquerel · 2024-09-20T17:48:18Z

crates/weaver_forge/templates/semconv_jq_fn/weaver.yaml

+      | map(. + {
+          event_namespace: (if .id | index(".") then (.id | split(".") | .[0:-1] | join(".")) else "other" end)
+        })
+      | sort_by(.event_namespace, .id)


Just a question: Why use event_namespace instead of root_namespace, as with the other signals? I just want to avoid the same kind of issues we had in the past with name and metric_name, which made it harder to generalize without providing significantly higher value for the UX.

Not blocking.

+1 to this from me. We should avoid diverging here, although IIUC this is just testing code right?

We can clean this up later

Yes, it is just testing code. And for events the entire "event_namespace" is important and not just the "root_namespace"...

crates/weaver_resolver/src/lib.rs

lquerel

Once the merge with the other PR (AnyValueSpec) is done, and the few minor corrections mentioned in my comments are made, I see no problem with creating a release with this PR. Please update the CHANGELOG.md to announce the support of events in Weaver (section [Next]).

However, as a precaution, I think it would be a good idea to check the backward compatibility with the older registries. The previous version was able to check all versions from 1.21.0 to the latest one. You can test each version with the following command line.

cargo run -- registry check -r https://github.com/open-telemetry/semantic-conventions/archive/refs/tags/v1.21.0.zip[model]

Thanks for all your hard work on this!

lquerel · 2024-09-20T17:59:09Z

crates/weaver_semconv/src/attribute.rs

@@ -490,6 +491,73 @@ impl Examples {
            ),
        }
    }
+
+    /// Validation logic for the any_value.
+    pub(crate) fn validate_any_value(


Why is this function in the attribute.rs file instead of any_value.rs?

I kept it here with the other "Examples" validate so that there was only one location for handling "Examples"...

Local backward compat check past with the expected warnings about examples SHOULD be of type string[]

Ok. So we are ready to go?

…resolved schema - simplify validate_any_value_examples - Reduce the test yaml events to smaller set - Removed observed file

MSNev requested review from lquerel and jsuereth as code owners August 7, 2024 16:37

MSNev mentioned this pull request Aug 7, 2024

draft: Initial Event body fields support #236

Closed

MSNev commented Aug 7, 2024

View reviewed changes

crates/weaver_resolver/data/registry-test-4-events/registry/browser-event.yaml Outdated Show resolved Hide resolved

jsuereth reviewed Aug 7, 2024

View reviewed changes

defaults/jq/semconv.jq Outdated Show resolved Hide resolved

jsuereth reviewed Aug 7, 2024

View reviewed changes

defaults/jq/semconv.jq Outdated Show resolved Hide resolved

jsuereth reviewed Aug 7, 2024

View reviewed changes

crates/weaver_semconv/src/group.rs Outdated Show resolved Hide resolved

lmolkova reviewed Aug 8, 2024

View reviewed changes

lquerel requested changes Aug 8, 2024

View reviewed changes

MSNev force-pushed the MSNev/EventBody branch from ec589fb to 4e9838d Compare August 9, 2024 22:00

github-advanced-security bot found potential problems Aug 9, 2024

View reviewed changes

MSNev force-pushed the MSNev/EventBody branch from 4e9838d to 46ca492 Compare August 9, 2024 22:24

github-advanced-security bot found potential problems Aug 9, 2024

View reviewed changes

crates/weaver_resolver/src/body.rs Fixed Show fixed Hide fixed

crates/weaver_resolver/src/body.rs Fixed Show fixed Hide fixed

MSNev commented Aug 9, 2024

View reviewed changes

crates/weaver_semconv/src/body.rs Outdated Show resolved Hide resolved

MSNev force-pushed the MSNev/EventBody branch 3 times, most recently from 5111ee7 to 42b7a6c Compare August 9, 2024 22:44

trisch-me requested a review from a team August 12, 2024 14:48

MSNev force-pushed the MSNev/EventBody branch from 359d925 to c9c9738 Compare August 12, 2024 15:50

MSNev requested review from lquerel, lmolkova and jsuereth August 12, 2024 16:48

MSNev force-pushed the MSNev/EventBody branch 4 times, most recently from 6832983 to 88ac690 Compare August 14, 2024 14:04

lmolkova reviewed Aug 16, 2024

View reviewed changes

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

MSNev force-pushed the MSNev/EventBody branch from 88ac690 to 49a0231 Compare August 16, 2024 21:12

MSNev requested a review from a team August 16, 2024 21:13

jsuereth reviewed Sep 3, 2024

View reviewed changes

crates/weaver_semconv/src/body.rs Outdated Show resolved Hide resolved

MSNev force-pushed the MSNev/EventBody branch from f4a3c3e to 82ac35d Compare September 3, 2024 14:52

MSNev mentioned this pull request Sep 12, 2024

feat: Add AnyValue proto support to semconv crate only #365

Closed

MSNev force-pushed the MSNev/EventBody branch from 82ac35d to 785d84b Compare September 17, 2024 01:54

MSNev force-pushed the MSNev/EventBody branch from 785d84b to b8a97a6 Compare September 18, 2024 23:07

MSNev requested review from a team as code owners September 18, 2024 23:07

MSNev force-pushed the MSNev/EventBody branch from b8a97a6 to 2d7ad64 Compare September 18, 2024 23:10

MSNev requested review from lmolkova and jsuereth September 18, 2024 23:12

MSNev added 6 commits September 19, 2024 09:50

feat: Add support for event body fields

43d5e0d

feat: Add event body spec to schema files

2316b62

- address review comments

c10b396

Update body definition to include

160a9ad

- type - brief - note - stability - examples

Refactor Body to AnyValue

4223318

Remove allow_custom_values, deprecated and type_display

d836d08

MSNev force-pushed the MSNev/EventBody branch from 2d7ad64 to d836d08 Compare September 19, 2024 16:52

jsuereth approved these changes Sep 19, 2024

View reviewed changes

crates/weaver_semconv/src/group.rs Show resolved Hide resolved

crates/weaver_resolver/data/registry-test-4-events/registry/trace-gen-ai.yaml Outdated Show resolved Hide resolved

MSNev commented Sep 20, 2024

View reviewed changes

crates/weaver_resolver/data/registry-test-4-events/expected-registry.json Outdated Show resolved Hide resolved

MSNev commented Sep 20, 2024

View reviewed changes

crates/weaver_resolver/data/registry-test-4-events/expected-registry.json Outdated Show resolved Hide resolved

lquerel reviewed Sep 20, 2024

View reviewed changes

crates/weaver_resolver/data/registry-test-4-events/observed-events.json Outdated Show resolved Hide resolved

crates/weaver_resolver/data/registry-test-4-events/registry/trace-gen-ai.yaml Outdated Show resolved Hide resolved

lquerel reviewed Sep 20, 2024

View reviewed changes

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

lquerel approved these changes Sep 20, 2024

View reviewed changes

MSNev added 2 commits September 20, 2024 13:20

remove resolved and resolver any_value.rs just use AnyValueSpec in …

15ee5fb

…resolved schema - simplify validate_any_value_examples - Reduce the test yaml events to smaller set - Removed observed file

remove UnresolvedBody error and update ChangeLog.md

a6ac3c1

lquerel merged commit 3192be4 into open-telemetry:main Sep 20, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for event body fields #297

feat: Add support for event body fields #297

MSNev commented Aug 7, 2024 •

edited

Loading

MSNev commented Aug 7, 2024

lquerel left a comment

MSNev commented Sep 17, 2024

MSNev commented Sep 18, 2024

jsuereth left a comment

lquerel left a comment

lquerel Sep 20, 2024

MSNev Sep 20, 2024 •

edited

Loading

lquerel Sep 20, 2024

jsuereth Sep 20, 2024

MSNev Sep 20, 2024

lquerel left a comment

lquerel Sep 20, 2024

MSNev Sep 20, 2024

MSNev Sep 20, 2024

lquerel Sep 20, 2024

feat: Add support for event body fields #297

feat: Add support for event body fields #297

Conversation

MSNev commented Aug 7, 2024 • edited Loading

MSNev commented Aug 7, 2024

lquerel left a comment

Choose a reason for hiding this comment

MSNev commented Sep 17, 2024

MSNev commented Sep 18, 2024

jsuereth left a comment

Choose a reason for hiding this comment

lquerel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSNev Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lquerel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSNev commented Aug 7, 2024 •

edited

Loading

MSNev Sep 20, 2024 •

edited

Loading