-
Notifications
You must be signed in to change notification settings - Fork 1.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
Closed
Closed
Flatten schema pkg #4538
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
fa227a3
Move v1.1 up to schema
MrAlias 5bb9a08
Move ast and types into schema
MrAlias 6f1ed64
Move parse validation into schema
MrAlias 51f9128
Remove v1.0
MrAlias b7aa728
Add example test
MrAlias a19f34b
Move README docs into pkg docs
MrAlias 78c8318
Fix import comment
MrAlias d479e47
Rename Transform to Changeset
MrAlias af5abaf
Replace SemConvVersion with a string
MrAlias 975ca51
Comment the Changeset fields
MrAlias c9efe30
Upgrade gopkg.io/yaml from v2 to v3 in schema (#4535)
MrAlias dcce489
Add changes to changelog
MrAlias 76eae4e
Add TODO for slimmed down semver.Version use
MrAlias 5e3813c
Merge branch 'main' into flatten-schema
MrAlias 37e2b48
Merge branch 'main' into flatten-schema
MrAlias bffcf25
Merge branch 'main' into flatten-schema
hanyuancheung fc0fda0
Remove breaking changes comment
MrAlias db29929
Merge branch 'main' into flatten-schema
MrAlias 6ef6e67
Replace FileFormat with FileFormatRange
MrAlias File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
// Package schema provides types and utilities used to interact with | ||
// [OpenTelemetry schema files]. | ||
// | ||
// [OpenTelemetry schema files]: https://github.com/open-telemetry/opentelemetry-specification/blob/007f415120090972e22a90afd499640321f160f3/specification/schemas/file_format_v1.1.0.md | ||
package schema // import "go.opentelemetry.io/otel/schema" | ||
|
||
import ( | ||
"io" | ||
"os" | ||
|
||
"gopkg.in/yaml.v3" | ||
) | ||
|
||
// ParseFile parses a Schema from the schema file found at path. | ||
func ParseFile(path string) (*Schema, error) { | ||
file, err := os.Open(path) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return Parse(file) | ||
} | ||
|
||
// Parse parses a Schema read from r. | ||
// | ||
// If r contains a Schema with a file format version outside FileFormatRange, | ||
// an error will be returned. | ||
// | ||
// 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) | ||
|
||
var s Schema | ||
if err := d.Decode(&s); err != nil { | ||
return nil, err | ||
} | ||
|
||
if err := s.validate(); err != nil { | ||
return nil, err | ||
} | ||
return &s, nil | ||
} |
Oops, something went wrong.
Oops, something went wrong.
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.
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: