Skip to content
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

Add ability to parse Schema files according to OTEP 0152 #2267

Merged
merged 14 commits into from Oct 15, 2021

Conversation

tigrannajaryan
Copy link
Member

The parser and parsed representation (AST) are placed in a separate
Go module so that they are can be consumed independently without
the need to bring the rest of the SDK.

Ability to use the parsed representation for schema conversions
can be added later.

@tigrannajaryan
Copy link
Member Author

@MrAlias @Aneurysm9 can you please have a look and tell me if this goes in the right direction?

@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

Merging #2267 (b25aa10) into main (478dc4f) will increase coverage by 0.0%.
The diff coverage is 93.3%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2267   +/-   ##
=====================================
  Coverage   72.6%   72.6%           
=====================================
  Files        170     171    +1     
  Lines      11873   11918   +45     
=====================================
+ Hits        8621    8660   +39     
- Misses      3019    3023    +4     
- Partials     233     235    +2     
Impacted Files Coverage Δ
schema/v1.0/parser.go 93.3% <93.3%> (ø)
...s/otlp/otlptrace/internal/connection/connection.go 14.6% <0.0%> (-1.6%) ⬇️

Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think this looks good. Making this it's own module sounds like a good idea and I like the clear layout of code.

How do plan to handle version increments? I see the tests fail for v2.* and v1.1.*, but not v1.0.1. I'm guessing for the patch version we expect to be compatible, I can see a v2 living in it's own package, but I'm wondering what the plan is for v1.1.* and the like.

@tigrannajaryan
Copy link
Member Author

How do plan to handle version increments? I see the tests fail for v2.* and v1.1.*, but not v1.0.1. I'm guessing for the patch version we expect to be compatible, I can see a v2 living in it's own package, but I'm wondering what the plan is for v1.1.* and the like.

Yes, v2 should live in its own package and patch numbers are ignored since they are compatible.

For minor number I think we can force the user to declare the maximum they expect. It is currently implied to be equal to 0, but we can make the Parse function accept maximum minor version number it expects, e.g.:

func Parse(schemaFile string, maxMinorVerNum int) (*ast.Schema, error) {
}

// in checkFileFormatField compare minor number with maxMinorVerNum.

Usage looks a bit ugly, not sure if we can make it nicer:

const maxMinorVer = 2;
ts, err := schema.Parse("myschema.yaml", maxMinorVer) // maximum version number we can handle is 1.2.x. 

Maybe we can change a bit with how we supply the max minor number to make the usage more natural. I can play with some possible approaches to see what I can come up with, but the idea is going to be the same: force the user to declare what maximum version they expect.

schema/parser.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Oct 2, 2021

I thought a bit more and I think we should just make all major and all minor versions separate packages. The initial version will be schema/v1.0, the next minor version will go in schema/v1.1, etc. This is similar to how we handle semconv packages and forces the user to explicitly choose the version they expect.

Using schema/v1.2 means that you are ok to see files in format 1.0, 1.1, 1.2, but not anything newer than 1.2.x (so range [1.0-1.3) is what you expect).

New versions of file format are going to be rare (maybe 1-2 minor versions per year, ever less major versions) so we are not going to have a proliferation of packages.
Let me refactor to show what it would look like.

@tigrannajaryan
Copy link
Member Author

@MrAlias Refactored. PTAL.

@tigrannajaryan
Copy link
Member Author

If we are happy with this direction, I will tidy up the draft for full review.

@tigrannajaryan tigrannajaryan force-pushed the schema-parser branch 2 times, most recently from ce49916 to d6e5d84 Compare October 4, 2021 14:42
@tigrannajaryan
Copy link
Member Author

Squashed and ready for review.

schema/v1.0/ast/ast_schema.go Outdated Show resolved Hide resolved
schema/v1.0/ast/ast_schema.go Outdated Show resolved Hide resolved
schema/v1.0/ast/logs.go Outdated Show resolved Hide resolved
schema/v1.0/ast/logs.go Outdated Show resolved Hide resolved
schema/v1.0/ast/metrics.go Outdated Show resolved Hide resolved
schema/v1.0/ast/spans.go Show resolved Hide resolved
@Aneurysm9
Copy link
Member

Please add the new module to versions.yaml. This should probably go in a new module set, perhaps experimental-schema, starting at v0.0.1.

@tigrannajaryan
Copy link
Member Author

Please add the new module to versions.yaml. This should probably go in a new module set, perhaps experimental-schema, starting at v0.0.1.

Done.

@tigrannajaryan
Copy link
Member Author

All OTEP fixes that this depended on are now merged. This is ready for final review and merging.

CHANGELOG.md Outdated Show resolved Hide resolved
schema/v1.0/ast/spans.go Outdated Show resolved Hide resolved
schema/v1.0/ast/ast_schema.go Outdated Show resolved Hide resolved
schema/go.mod Show resolved Hide resolved
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall 👍, nothing blocking.

schema/v1.0/ast/metrics.go Show resolved Hide resolved
schema/v1.0/ast/spans.go Outdated Show resolved Hide resolved
schema/v1.0/parser.go Outdated Show resolved Hide resolved
schema/v1.0/parser.go Outdated Show resolved Hide resolved
schema/v1.0/types/types.go Outdated Show resolved Hide resolved
schema/README.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member Author

@Aneurysm9 do you want to have another look or we can merge?

@tigrannajaryan
Copy link
Member Author

@open-telemetry/go-approvers is there anything else to do on this?

@MrAlias
Copy link
Contributor

MrAlias commented Oct 15, 2021

@open-telemetry/go-approvers is there anything else to do on this?

Looks good. Going to merge.

@MrAlias MrAlias merged commit e72a235 into open-telemetry:main Oct 15, 2021
@tigrannajaryan tigrannajaryan deleted the schema-parser branch October 15, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants