-
Notifications
You must be signed in to change notification settings - Fork 56
Improve the quality of error messages emitted by Weaver #759
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
Conversation
| /// Supported group types are: `attribute_group`, `span`, `event`, `metric`, `entity`, `scope`. | ||
| /// Mandatory fields are: `id` and `brief`. | ||
| /// | ||
| /// Note: The `resource` type is no longer used and is an alias for `entity`. |
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 like how this forces better rust documentation too.
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.
Yes, I’ve improved a few comments, but we’ll certainly need to continue this effort throughout the objects representing all the semantic conventions.
| // Force the `miette` context to 5 lines. | ||
| miette::set_hook(Box::new(|_| { | ||
| Box::new( | ||
| miette::MietteHandlerOpts::new() |
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.
It looks like just generally more investment in Miette would be valuable from what I see.
E.g. we should see if we can preserve spans further into resolution for error messages over time.
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 was going to say something similar I think - these errors are now better than the ones produced here: https://github.com/open-telemetry/weaver/blob/main/crates/weaver_semconv/src/group.rs#L107.
On the topic of using this machine generated json schema. The hand-crafted json schema: https://github.com/open-telemetry/weaver/blob/main/schemas/semconv.schema.json encodes more rules than can be expressed in the Rust structs/enum, I think. And it is aligned with --future as @lmolkova pointed out. So it would be a shame to lose this. An author with a modern text editor with the schema plugged in will be warned there and then that prefix is not part of the schema. With the machine generated version there won't be this warning until they pass it through weaver. This IMO creates a disconnected experience for the author where they'll have to go back and forth between command line and text to make their yaml compliant.
The deserialization of YAML semconv files using
serdeoften results in low-quality error messages.This PR:
serdefor deserialization.serdedetects errors:SemConvSpecstructure.This PR also adds a new parameter to the command
weaver registry json-schemato generate the JSON Schema for thesemconv-groupinstead of the resolved registry.Example of error messages:
Closes: #746