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

Generate classes from json schema, demo parsing #46

Closed
wants to merge 1 commit into from

Conversation

jack-berg
Copy link
Collaborator

No description provided.

"jaeger": {
"$ref": "#/definitions/Jaeger"
}
},
"patternProperties": {
Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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": {
Copy link
Collaborator Author

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.

Copy link
Collaborator

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

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.

2 participants