-
Notifications
You must be signed in to change notification settings - Fork 273
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
Mostly fix json schema generation for otel config. #607
Conversation
This is such a big part of the config that it's worth having. It's possible that the schema is a little too lenient, and would allow invalid config for custom attributes or headers, but mostly it is OK.
This comment has been minimized.
This comment has been minimized.
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.
Naming is hard...
#[derive(JsonSchema)] | ||
#[allow(dead_code)] | ||
enum ProtocolMirror { | ||
/// GRPC protocol | ||
Grpc, | ||
// HttpJson, | ||
/// HTTP protocol with binary protobuf | ||
HttpBinary, | ||
} | ||
|
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.
NIT: I presume this is a restricted copy of the telemetry Protocol struct which only contains the protocols we support.
In which case, maybe rename to ImplementedProtocol
or RestrictedProtocol
or ProtocolSubset
...?
(It's definitely not a Mirror and that name doesn't give much of a hint about its purpose.)
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's not even that, it is an exact copy of the otel struct. I'll add a comment though.
Add comment to say what a mirror enum is used for.
This is such a big part of the config that it's worth having.
Previously we were short circuiting the schema mechanism as there various types are reused from Otel and these do not have schema generation.
It's possible that the schema is a little too lenient, and would allow invalid config for custom attributes or headers, but mostly it is OK.