-
Notifications
You must be signed in to change notification settings - Fork 7
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
Generate classes from json schema, demo parsing #46
Conversation
"jaeger": { | ||
"$ref": "#/definitions/Jaeger" | ||
} | ||
}, | ||
"patternProperties": { |
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.
Generated classes aren't able to parse properties based on patterns. The code interpreting the parsed results will have to manually detect these patterns and parse the results as the corresponding type. We should minimize use of patternProperties to reduce this extra work.
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 wonder if there's a way to define an unmarshaling code in one place through some interface that can then be re-used. The collector does this by defining a component.ID
that handles the parsing.
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 manage to customize the code generation such that a hook can be added for custom deserialization. This solves the problem, but I came across other issues in the process:
- The code generation tool I used doesn't generate classes for definitions that aren't referenced, and it appears that pattern property references to the otlp, jaeger, and zipkin don't count. To solve this I had to break out those definitions into standalone files.
- Once some of the definitions were split out from a single file, I had to tell the validation tool how to resolve URIs to definitions that live outside the file.
I imagine implementations in other languages will encounter similar issues.
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.
So embedded reference definitions don't work but reference definitions in separate files does?
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.
Yup. There's an issue tracking it, but its been stagnant for a couple of years now. This comment explains it:
When jsonschema2pojo was created, 'definitions' was not part of the standard. Many people started using 'definitions' to hold extra schemas but we didn't add support here because it was just a common pattern but not part of the standard.
@@ -175,7 +175,7 @@ | |||
], | |||
"title": "Processor" | |||
}, | |||
"LogRecordProcessorArgs": { | |||
"ProcessorArgs": { |
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.
Should we have a general ProcessorArgs
type, or separate the types for LogRecordProcessor
and SpanProcessor
?
Also, the properties here are specific to batch processor. Will need to rethink how to define this type to accommodate simple, batch, and extension processors.
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 think ProcessorArgs is ok until we have a reason to be more specific.
I suspect we'll want per type of processor arguments, much like we'll want the same for exporters. Though I guess that will only support the known processor/exporter. We should plan to provide documentation on how one would define and publish their own schemas if they wanted to support additional components
No description provided.