-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Flatten schema pkg #4538
Flatten schema pkg #4538
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4538 +/- ##
=====================================
Coverage 82.3% 82.3%
=====================================
Files 226 227 +1
Lines 18557 18523 -34
=====================================
- Hits 15286 15258 -28
+ Misses 2983 2979 -4
+ Partials 288 286 -2
|
dfd49dd
to
76eae4e
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.
I did my best to make a high-level design review first.
// If r contains an invalid schema URL an error will be returned. | ||
func Parse(r io.Reader) (*Schema, error) { | ||
d := yaml.NewDecoder(r) | ||
d.KnownFields(true) |
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.
Putting one long comment with many remarks and thoughts as I find them very related to each other.
I have some problems with the parsing logic and interpreting the specification.
Extensibility?
From stable spec:
the schema file format is extensible.
What does it mean? Can we add custom fields that may be added in future versions to forward compatibility? Should consider removing this line? On the other hand, the author maybe just wanted to say that the file_format may evolve in future version (and the "extensibility" term is not used correctly/precisely). Extensibility in "formats" usually means that custom things can be added thanks to some extension points e.g. https://www.w3schools.com/xml/el_extension.asp. As far as I see our current implementation uses d.KnownFields(true)
. It errors when encounters unknown fields for the currently supported version. Our implementation does not offer forward compatibility. In my opinion, our implementation is OK and the problem is that the specification is not clear. Since it lacks normative wording, we may leave it as is.
Parsing logic vs file_format version
I noticed that our implementation will NOT throw an error when the file has file_format: 1.0.0
, but uses a field introduced from 1.1.0
version. For example, if you change file_format
to 1.0.0
in valid-example.yaml
the ParseFile
will successfully parse without any problem. Take notice that, the experimental (sic!) file format says:
Defines the file format. MUST be set to 1.1.0.
Therefore, I am not sure if the current implementation complies with the specification.
If I understand correctly, if we would like to be so strict, we would need to have dedicated structs for each minor version. I find it not pragmatic (an overkill). I would NOT like doing it and I would rather change the specification (if needed) than our implementation.
For reference let me try to describe the parsing strategy that browsers and Docker Compose have for parser HTML/YAML files.
- Parser just interprets the "major" version. The minor version specified by the user is "ignored" by the user. It is only informative for humans (what is more, the latest Docker version even ignores the version field and uses only the latest version of the schema when parsing).
- Parser emit warnings (errors that can be ignored) in case of unknown fields that it does not understand. However, it should still return the parsed object (forward compatible). The parser could also silently ignore unknown fields.
In my opinion, the current implementation is OK. Right now we simply parse against the latest supported version. We can decide later what we will do when v2
of file_version
is defined.
Possible safe forward compatibility
If r contains a Schema with a file format version higher than FileFormat, an error will be returned.
We could still parse a schema if it does NOT use any new field (a kind of forward compatibility which offers support until does NOT use unknown features). I think we could offer this later as this would not be a breaking change. I would view it as an enhancement. However, it is YAGNI we will have to bump the file_version support as soon as new OTel semconv schema uses the file_version.
Summary
I like the current design and implementation. However, it would be good to ensure that the parsing implementation complies with the specification. Especially the part that we are able to parse files which have file_format: 1.0.0
while also having syntax from a newer format e.g. 1.1.0
. In my opinion, the current implementation is OK. I believe it would be more practical to clarify or modify the specification instead of altering the proposed implementation.
It also worth to notice that any validation logic changes will affect https://github.com/open-telemetry/build-tools/tree/main/schemas. As far as I reviewed, the current PR does NOT introduce any changes in the parsing+validation logic.
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 had considered restricting parsing to only allow parsing of a specific version of the file format, but I don't think that's the user experience we want to provide. If a user has a schema that incorrectly states it is 1.0.0 and uses 1.1.0 fields, I would rather produce a valid schema for the user. I doubt they care that the schema was incorrect, they just want the data.
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 agree. Should we add tests for this behavior and should it be documented?
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 agree. Should we add tests for this behavior and should it be documented?
Yeah, that sounds good to me. I'll update in a bit.
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.
There is another section in the file format that specifies that this is the intended behavior:
Correspondingly:
Consumers of the schema file SHOULD NOT attempt to interpret the schema file if the MAJOR version number is different (higher or lower) than what the consumer supports. Consumers of the schema file SHOULD NOT attempt to interpret the schema file if the MINOR version number is higher than what the consumer supports. Consumers MAY ignore the PATCH number.
schema/schema.go
Outdated
return fmt.Errorf("invalid file format version: %q: %w", s.FileFormat, err) | ||
} | ||
|
||
if FileFormat.LessThan(ffVer) { |
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.
How about adding a validation that ensures that file_version
is greater or equal than 1.0.0
?
This would protect us agains 0.x.y
and also same pattern could be used when (if) we add support for 2.x.y
.
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.
A README.md would be useful.
|
5de3aa9
to
6ef6e67
Compare
Closing. Not needed: #4503 (comment) |
Fixes #4534
schema/v1.0
andschema/v1.1
packages intoschema
.schema
package parse the latest file format and all versions less than that into the same structv2
ofschema
All
andResource
types to replace the genericAttributes
type that was used by bothVersionDef
toChangeset
to better match its purpose as the differences between two successive versions