-
Notifications
You must be signed in to change notification settings - Fork 167
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 custom serialized for last_error, update schemas #1140
Conversation
28171d2
to
09a2629
Compare
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
09a2629
to
6004824
Compare
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.
Overall, good PR.
Some changes and nits requested.
@@ -177,13 +170,59 @@ pub struct Stats { | |||
#[serde(default, with = "serde_nanos")] | |||
pub average_processing_time: std::time::Duration, | |||
/// Last error that occurred. | |||
#[serde(with = "serde_error_string")] |
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.
maybe we should add skip_serializing_if = "Option::is_none"
?
Or with experience when working on cross compatibility tests its better to serialize to ""?
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 it's a good idea, although it is required in json schema: https://github.com/nats-io/jsm.go/blob/main/schemas/micro/v1/stats_response.json
On the other hand, it's marked as optional in ADR. I would say it should be optional and we should update the schema, then we can add skip_serializing_if
here. What do you think?
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
93f6a52
to
3678346
Compare
…/nats.rs into service-api-config-serde
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
8e3e18b
to
9016d38
Compare
/// Queue group to which this endpoint is assigned to. | ||
pub queue_group: String, | ||
} | ||
|
||
mod serde_error_string { |
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.
As we have our own error type, the serde impl can be attached to it directly.
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.
as discussed, as a first pass, this is ok, as we would need some custom logic for serializing into "" instead of null in case of None etc.
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.
LGTM!
async_nats::service::error::Error
to properly format error stringPing
as a separate struct and addedmetadata
to itSigned-off-by: Piotr Piotrowski piotr@synadia.com