-
Notifications
You must be signed in to change notification settings - Fork 53
[WIP] V2 Resolved Schema #980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments, but looks great overall!
crates/weaver_forge/src/lib.rs
Outdated
|
|
||
| assert!(diff_dir("expected_output/test", "observed_output/test").unwrap()); | ||
|
|
||
| // TODO - Remove this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it ok to remove now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still using this to test/demo. I was going to remove after we lock-in on the details of the schema.
| instrument: metric.instrument, | ||
| unit: metric.unit, | ||
| attributes, | ||
| entity_associations: metric.entity_associations, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not blocking, we can follow-up: we're checking that attributes exist, but not checking it for entities, it seems inconsistent.
| /// A list of span signal definitions. | ||
| pub spans: Vec<Span>, | ||
|
|
||
| /// A list of metric signal definitions. | ||
| pub metrics: Vec<Metric>, | ||
|
|
||
| /// A list of event signal definitions. | ||
| pub events: Vec<Event>, | ||
|
|
||
| /// A list of entity signal definitions. | ||
| pub entities: Vec<Entity>, | ||
|
|
||
| /// A list of span refinements. | ||
| pub span_refinements: Vec<SpanRefinement>, | ||
|
|
||
| /// A list of metric refinements. | ||
| pub metric_refinements: Vec<MetricRefinement>, | ||
|
|
||
| /// A list of event refinements. | ||
| pub event_refinements: Vec<EventRefinement>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be beneficial to group the definitions based on the base namespace ie db then at the top level we just have an array of namespaces. This concept and the use case for namespaces is described in more detail in https://github.com/open-telemetry/weaver/pull/867/files but I would start off with just the name + signals being the only properties of the namespace as the rest could be added via non-breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not convinced. I see the registry itself as a namespace going forward, but this is something that needs more discussion going forward.
e.g. in instance where we want a "bundle of things" would it be easier to create a registry to describe that bundle? That registry would come with some nice properties, like versioning, isolated codegen, docs, etc.
I think I'd like to move to point where, e.g. an implementation specific registry exists in OTEL for any implementation. I think this may give us some nice properties:
- Implementation-specific concerns can live near the instrumentation
- Implementations can document exactly what they provide, while guaranteeing conformance to semconv
- Implementations would document semconv version explicitly via their dependency to semconv repo. We could allow these to "hold back" while we do major evolutions (e.g. current RPC efforts).
There's obviously downsides too, but it's something I'd like us to consider / think through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what you are thinking with regards to implementations being their own registry, but I am torn about the core sem conv registry being split. I can see it helping but also making things harder. Let's discuss implementations as standalone registry in a seperate thread.
If we look at how the namespace value would be used in the core registry, it would enable a single entry point to be provided for the namespace ie db which lists everything in that namespace rather than splitting it into a seperate entry point per signal type like we do now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, it took me quite some time to go through all of this. Thanks for the massive refactoring of the semconv schema and everything that comes with it!
Sorry for the number of comments. A lot of them are just copy/paste issues or typos. There are probably only a handful that really concern the approach or the decisions made. Apologies in advance if some of these points or answers have already been discussed in the SIG meetings; I haven't been very present in those lately.
crates/weaver_forge/src/v2/entity.rs
Outdated
| /// The type of the entity. | ||
| pub r#type: SignalId, | ||
|
|
||
| /// List of attributes that belong to this event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment would benefit from being updated to match the field below :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I was rushing for the demo, I'll clean all these up - Didn't want this to be a full review, more of "check the direction" review.
I think the discussion on Attribute/AttributeRef is the thing to focus on in this review :)
| if let Some(a) = v2_catalog.convert_ref(attr) { | ||
| span_attributes.push(span::SpanAttributeRef { | ||
| base: a, | ||
| requirement_level: attr.requirement_level.clone(), | ||
| sampling_relevant: attr.sampling_relevant.clone(), | ||
| }); | ||
| } else { | ||
| // TODO logic error! | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dance, attribute-ref -> attribute -> attribute-ref, necessary because the comparison rules for attributes differ between v1 and v2?
That brings us back to my previous question about the rationale behind this difference. In any case, once we've fully migrated to v2, we shouldn't have this kind of thing anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - this actually leads to many less attributes in the catalog, but the catalog now matches almost exactly what we get in Semconv attribute registry.
A lot of this dance can be fully removed when we're directly doing resolve on V2.
| /// the attribute is "recommended". When set to | ||
| /// "conditionally_required", the string provided as <condition> MUST | ||
| /// specify the conditions under which the attribute is required. | ||
| pub requirement_level: RequirementLevel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admit I'm not entirely clear on why requirement_level gets special treatment here. In what way is requirement_level more event-specific than note, examples, ..., in the context of a refinement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking of this via the "attribute registry" we have in otel.
Requirement level isn't an independent concept for any individual attribute, while examples, note is.
Requirement level only makes sense in the context of a specific signal (e.g. this metric requires this attribute). Additionally, requirement level is almost guaranteed to diverge between Metrics<->Spans/Events in some critical ways, due to cardinality constraints between the signals.
If you look at the existing semconv registries (e.g. attributes), you'll see we don't include requirement_level.
I feel pretty strongly we should NOT include requirement_level in either the attribute catalog/registry or outside of a signal context.
…pans, no real tests.
…creating V2 schema.
67e6762 to
209883f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #980 +/- ##
=======================================
- Coverage 78.3% 75.2% -3.1%
=======================================
Files 77 82 +5
Lines 6122 6559 +437
=======================================
+ Hits 4795 4934 +139
- Misses 1327 1625 +298 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…vert resolved schema to v2.
|
testing this out on semconv and checking against problems identified for v1 in open-telemetry/semantic-conventions#2469 Resolved schema v2 - https://gist.githubusercontent.com/lmolkova/34dc5c0b0f583ca80681af3c9334238d/raw/d5b1ce469f149c1586b277cf3fe0f5772a480b72/semconv_schema_v2_copy.yaml works fine now ! |
This comment was marked as resolved.
This comment was marked as resolved.
| value: None, | ||
| role: None, | ||
| }); | ||
| assert_eq!(result.is_some(), true); |
Check failure
Code scanning / clippy
used assert_eq! with a literal bool Error
| value: None, | ||
| role: None, | ||
| }); | ||
| assert_eq!(result2.is_some(), true); |
Check failure
Code scanning / clippy
used assert_eq! with a literal bool Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested weaver registry resolve --v2 and it works. I'm in favor of merging it and following up
DO NOT MERGE: This is a work in progress for discussion.
This modifies the resolved telemetry schema (and template values) to look like the V2 definition schema.
With this PR you can resolve into the new schema via a new flag to check how it looks, e.g.
Here's a sample of the new layout: