-
Notifications
You must be signed in to change notification settings - Fork 72
Fix resolution for dependencies #1280
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,6 @@ use crate::catalog::Catalog; | |
| use crate::instrumentation_library::InstrumentationLibrary; | ||
| use crate::registry::{Group, Registry}; | ||
| use crate::resource::Resource; | ||
| use schemars::JsonSchema; | ||
| use serde::Serialize; | ||
| use std::collections::HashMap; | ||
| use weaver_semconv::deprecated::Deprecated; | ||
|
|
@@ -46,8 +45,7 @@ pub(crate) const V2_RESOLVED_FILE_FORMAT: &str = "resolved/2.0.0"; | |
| /// A Resolved Telemetry Schema. | ||
| /// A Resolved Telemetry Schema is self-contained and doesn't contain any | ||
| /// external references to other schemas or semantic conventions. | ||
| #[derive(Debug, JsonSchema)] | ||
| #[serde(deny_unknown_fields)] | ||
| #[derive(Debug)] | ||
| pub struct ResolvedTelemetrySchema { | ||
| /// Version of the file structure. | ||
| pub file_format: String, | ||
|
|
@@ -61,22 +59,18 @@ pub struct ResolvedTelemetrySchema { | |
| /// and signals. | ||
| pub catalog: Catalog, | ||
| /// Resource definition (only for application). | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this intentional?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, it does not need json schema - we don't (and don't need to) support it for v1. And it's not (de)serializable either
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to remove serialization from things that are not intended to be read or written by weaver. This helps to understand what's internal detail and what's a real contract |
||
| pub resource: Option<Resource>, | ||
| /// Definition of the instrumentation library for the instrumented application or library. | ||
| /// Or none if the resolved telemetry schema represents a semantic convention registry. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub instrumentation_library: Option<InstrumentationLibrary>, | ||
| /// The list of dependencies of the current instrumentation application or library. | ||
| #[serde(skip_serializing_if = "Vec::is_empty")] | ||
| pub dependencies: Vec<InstrumentationLibrary>, | ||
| /// Definitions for each schema version in this family. | ||
| /// Note: the ordering of versions is defined according to semver | ||
| /// version number ordering rules. | ||
| /// This section is described in more details in the OTEP 0152 and in a dedicated | ||
| /// section below. | ||
| /// <https://github.com/open-telemetry/oteps/blob/main/text/0152-telemetry-schemas.md> | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub versions: Option<Versions>, | ||
| /// The manifest of the registry. | ||
| pub registry_manifest: Option<RegistryManifest>, | ||
|
|
@@ -117,7 +111,12 @@ impl ResolvedTelemetrySchema { | |
| attrs: [Attribute; N], | ||
| deprecated: Option<Deprecated>, | ||
| ) { | ||
| let attr_refs = self.catalog.add_attributes(attrs); | ||
| let mut builder = catalog::test_utils::CatalogBuilder::from_catalog(&self.catalog); | ||
| let attr_refs: Vec<attribute::AttributeRef> = attrs | ||
| .into_iter() | ||
| .map(|a| builder.add(a, Some(group_id))) | ||
| .collect(); | ||
| self.catalog = builder.build(); | ||
| self.registry.groups.push(Group { | ||
| id: group_id.to_owned(), | ||
| r#type: GroupType::Metric, | ||
|
|
@@ -161,7 +160,12 @@ impl ResolvedTelemetrySchema { | |
| let al = AttributeLineage::new(group_id); | ||
| lineage.add_attribute_lineage(attr.name.clone(), al); | ||
| } | ||
| let attr_refs: Vec<attribute::AttributeRef> = self.catalog.add_attributes(attrs); | ||
| let mut builder = catalog::test_utils::CatalogBuilder::from_catalog(&self.catalog); | ||
| let attr_refs: Vec<attribute::AttributeRef> = attrs | ||
| .into_iter() | ||
| .map(|a| builder.add(a, Some(group_id))) | ||
| .collect(); | ||
| self.catalog = builder.build(); | ||
| self.registry.groups.push(Group { | ||
| id: group_id.to_owned(), | ||
| r#type: GroupType::AttributeGroup, | ||
|
|
@@ -525,20 +529,9 @@ impl ResolvedTelemetrySchema { | |
| mod tests { | ||
| use crate::attribute::Attribute; | ||
| use crate::ResolvedTelemetrySchema; | ||
| use schemars::schema_for; | ||
| use serde_json::to_string_pretty; | ||
| use weaver_semconv::deprecated::Deprecated; | ||
| use weaver_version::schema_changes::{SchemaItemChange, SchemaItemType}; | ||
|
|
||
| #[test] | ||
| fn test_json_schema_gen() { | ||
| // Ensure the JSON schema can be generated for the ResolvedTelemetrySchema | ||
| let schema = schema_for!(ResolvedTelemetrySchema); | ||
|
|
||
| // Ensure the schema can be serialized to a string | ||
| assert!(to_string_pretty(&schema).is_ok()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_diff() { | ||
| let mut prior_schema = ResolvedTelemetrySchema::new("1.0", "", ""); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
this is the real change for v1, let's populate root_attributes directly instead of trying to use lineage to infer if something in the catalog is original definition.
The rest in this file is cleanup - this struct should not allow to mutate attrs, does not need to be (de)serializable, many of its features were only used in tests