Allow resolving from resolved schema using Manifest file.#1160
Allow resolving from resolved schema using Manifest file.#1160jsuereth wants to merge 14 commits intoopen-telemetry:mainfrom
Conversation
…han just information on dependencies
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1160 +/- ##
=======================================
+ Coverage 80.1% 80.4% +0.3%
=======================================
Files 108 108
Lines 8440 8661 +221
=======================================
+ Hits 6763 6967 +204
- Misses 1677 1694 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…st multi-dependency test.
jsuereth
changed the title
jerbly
left a comment
jerbly
left a comment
There was a problem hiding this comment.
This is a big one... I'm going to try this branch out. In the meantime, I found a couple of bugs and some nits.
| .map(|ar| { | ||
| // TODO - this should be non-panic errors. | ||
| let attr = self | ||
| .attribute_catalog | ||
| .attribute(&ar.base) | ||
| .expect("Unable to find attr on catalog, invalid registry!"); | ||
| attribute_catalog.attribute_ref(Attribute { | ||
| name: attr.key.clone(), | ||
| r#type: attr.r#type.clone(), | ||
| brief: attr.common.brief.clone(), | ||
| examples: attr.examples.clone(), | ||
| tag: None, | ||
| requirement_level: ar.requirement_level.clone(), | ||
| sampling_relevant: None, | ||
| note: attr.common.note.clone(), | ||
| stability: Some(attr.common.stability.clone()), | ||
| deprecated: attr.common.deprecated.clone(), | ||
| prefix: false, | ||
| tags: None, | ||
| annotations: Some(attr.common.annotations.clone()), | ||
| value: None, | ||
| role: None, | ||
| }) |
There was a problem hiding this comment.
nit: a helper function would be good for this, it's repeated a few times.
| let result = f(sub_folder.unwrap_or("".to_owned())); | ||
| if result.is_empty() { | ||
| None | ||
| } else { | ||
| Some(result) | ||
| } |
There was a problem hiding this comment.
nit: this could be extracted to a function - it's duplicated.
double-nit: unwrap_or("".to_owned()) can be unwrap_or_default()
| /// Reads a serialized object with serde from the given virtual directory path. | ||
| fn from_vdir<T: serde::de::DeserializeOwned>(f: &VirtualDirectoryPath) -> Result<T, Error> { | ||
| let path = VirtualDirectory::try_new(f).map_err(|e| Error::InvalidUrl { | ||
| url: format!("{f}"), |
There was a problem hiding this comment.
| url: format!("{f}"), | |
| url: f.to_string(), |
| } | ||
| Group { | ||
| id: e.id().to_owned(), | ||
| r#type: weaver_semconv::group::GroupType::Event, |
There was a problem hiding this comment.
| r#type: weaver_semconv::group::GroupType::Event, | |
| r#type: weaver_semconv::group::GroupType::Entity, |
| Some(self.registry.vdir_path().map_sub_folder(|path| { | ||
| if vdir_was_manifest_file { | ||
| match Path::new(&path).parent() { | ||
| Some(parent) => format!("{}/{resolved_url}", parent.display()), |
There was a problem hiding this comment.
Is this right? parent.display() will use os dependent separators, so backslashes in Windows.
| /// There was an issue resolving definition schema. | ||
| #[error(transparent)] | ||
| #[diagnostic(transparent)] | ||
| FailToResolveDefinition(#[from] weaver_semconv::Error), |
There was a problem hiding this comment.
The #[from] here will make any weaver_semconv::Error a FailToResolveDefinition error even if it has nothing to do with resolving definitions.
2 participants
Fixes #1134
Should allow specifying a schema like in OTEP 4815.
http://my-schema.org/manifest/1.0.0resolved_schema_url, if available, on that manifest to resolve schema instead of grabbing.yamlfiles from the directory.resolved_schema_url, although this needs to be tested a bit.Thing not working yet:
manifest.yamlvs.registry_manifest.yamlas per OTEP 4815.