Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 14 commits
fa227a3
5bb9a08
6f1ed64
51f9128
b7aa728
a19f34b
78c8318
d479e47
af5abaf
975ca51
c9efe30
dcce489
76eae4e
5e3813c
37e2b48
bffcf25
fc0fda0
db29929
6ef6e67
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
Check warning on line 36 in schema/parser.go
Codecov / codecov/patch
schema/parser.go#L35-L36
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:
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 from1.1.0
version. For example, if you changefile_format
to1.0.0
invalid-example.yaml
theParseFile
will successfully parse without any problem. Take notice that, the experimental (sic!) file format says: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.
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
offile_version
is defined.Possible safe forward compatibility
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.
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:
Check warning on line 57 in schema/parser.go
Codecov / codecov/patch
schema/parser.go#L56-L57