Replace version in definition schema with file_format#1154
Replace version in definition schema with file_format#1154lmolkova wants to merge 7 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1154 +/- ##
=====================================
Coverage 79.9% 80.0%
=====================================
Files 109 109
Lines 8528 8528
=====================================
+ Hits 6820 6823 +3
+ Misses 1708 1705 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -1,4 +1,4 @@ | |||
| version: "2" | |||
| file_format: definition/2.0.0 | |||
There was a problem hiding this comment.
Nit: I'm not sure we want a three-version rule on language syntax. Or at least we should answer some questions:
- When do we fail to load a file because of major/minor versions? E.g. do we need something in place where weaver will fail to resolve
definition/2.1.0right now because we haven't defined the syntax there? - When will we bump this version? Should we do so on any major feature addition to the syntax? (probably). We should open a ticket to have some kind of automated tracker to let us know to do this so it doesn't get lost.
- Do we need a tool to migrate from version 2.x -> 2.y? I had started working on such a thing for 1->2.
I'm on board moving this direction of using file_format everywhere, and we can make these decisions over time, but I'd like to write down our versioning/loading policy a bit more (it can be in follow on bugs) and make sure our release that uses this syntax can "scale" to future versions/usage.
There was a problem hiding this comment.
That's a great point. I don't really see a point in following semver for the definition. I hope we'll deprecate v1 and remove support for it at some point, then this won't be necessary at all.
But I still want a bit of consistency with file_format (primarily i think version is ambiguous) , so I updated the PR to dofile_format: definition/2.
crates/weaver_semconv/src/semconv.rs
Outdated
| V1(SemConvSpecV1), | ||
| /// Version 2 of the semantic convention schema. | ||
| #[serde(rename = "2")] | ||
| #[serde(rename = "definition/2.0.0")] |
There was a problem hiding this comment.
This will ONLY handle the literal string definition/2.0.0. If we were to try a version bump, like definition/2.1.0 then this would fail to resolve previous versions.
I think there's some HARD implications here for us to sort through. E.g. I'd prefer If we had a custom deserialization path for SERDE that would resolve version string and then delegate to a deserializer of choice.
That custom path would be responsible for
- knowing if the version that came is compatible with versions we can represent in the current weaver.
- Choosing the right Rust structure to deserialize into.
lmolkova
force-pushed
the
definition-file-format
branch
from
f400877 to
3b3e3c2
Compare
|
|
||
| let cleaned = serde_yaml::Value::Mapping(mapping); | ||
| if is_v2 { | ||
| let v2 = serde_yaml::from_value::<SemConvSpecV2>(cleaned).map_err(de::Error::custom)?; |
There was a problem hiding this comment.
Can we make sure the errors are still meaningful here?
jsuereth
moved this from To consider for the next release
to Next Release
in OTel Weaver Project
jsuereth
left a comment
jsuereth
left a comment
There was a problem hiding this comment.
Overall looks good, two major questions:
- One on errors and what they look like wrapped now
- Another on whether we should update the file format error message
| /// This indicates the file version used is not yet stable. | ||
| #[error("Version `{version}` schema file format is not yet stable: {provenance}")] | ||
| /// This indicates the file format (version) used is not yet stable. | ||
| #[error("Version `{file_format}` schema file format is not yet stable: {provenance}")] |
There was a problem hiding this comment.
Nit: Should this now read "File Format"?
2 participants
Align definition schema marker with resolved schema introduced in #1136
Extracted from #1106