improve crashtracker config deserialization #588
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenchmarksComparisonBenchmark execution time: 2024-08-16 20:01:22 Comparing candidate commit abdd41e in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 50 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #588 +/- ##
==========================================
+ Coverage 71.77% 71.84% +0.07%
==========================================
Files 237 237
Lines 32871 32873 +2
==========================================
+ Hits 23594 23619 +25
+ Misses 9277 9254 -23
|
rochdev
force-pushed
the
rochdev/crashtracker-config-serde
branch
from
7fbf47a
to
614feba
Compare
rochdev
force-pushed
the
rochdev/crashtracker-config-serde
branch
from
395ef72
to
1c0db69
Compare
r1viollet
reviewed
Aug 20, 2024
| @@ -3,7 +3,7 @@ | |||
| use ddcommon::tag::Tag; | |||
| use serde::{Deserialize, Serialize}; | |||
|
|
|||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | |||
| #[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)] | |||
| pub struct CrashtrackerMetadata { | |||
There was a problem hiding this comment.
I do not think we would want this to have empty defaults. Any reason to allow this ?
There was a problem hiding this comment.
Why not if there are already defaults outside of Serde and they work 🤔
There was a problem hiding this comment.
What I meant is that you would want the user to fill some of these fields (like family). Feel free to ignore I'm not sure how you would make that field mandatory.
There was a problem hiding this comment.
That's the main thing. I think a configuration object struct should accept basically anything, and it should be up to the implementation using it to validate if the fields have values that are valid for whatever class using the object. In that sense, I think having a default value for every field makes sense, and simplifies usage by language-specific bindings.
There was a problem hiding this comment.
I disagree, in this case the default value is not valid and we should make it invalid by construction rather than validation.
For instance family really should be an enum, although right now it's more practical for it to be a String.
People should think about what they want in their metadata and deriving Default automatically goes against that
There was a problem hiding this comment.
I was replying basically for all 3 objects, but for CrashtrackerMetadata I tend to agree that most fields are mandatory, I added Default basically because I was doing it also for the other ones. I'll change that one so that only tags are optional.
r1viollet
reviewed
Aug 20, 2024
|
Most of the testing would be through unit tests. Though there are some end to end FFI tests that could be leveraged for crashtracking. |
It doesn't look like there are existing tests for crashtracking to begin with. And FFI tests are something else completely as Node cannot use FFI. |
r1viollet
reviewed
Aug 21, 2024
| if let Some(v) = uri.path_and_query { | ||
| builder = builder.path_and_query(v.deref()); | ||
| match StringOrSerializedUri::deserialize(deserializer)? { | ||
| StringOrSerializedUri::String(str) => str.parse().map_err(Error::custom), |
There was a problem hiding this comment.
are there ways to add a message here to differentiate between the errors ?
There was a problem hiding this comment.
I think the original error message will be properly remapped onto the new custom error, or at least that is my understanding of what this is doing"
If adding a rust unit test is difficult, ping me, I can approve to unblock. I agree the ffi tests won't help. |
Is there an example anywhere in the project where serialization/deserialization is tested that I can look at? |
Yes, I think this is an example |
| @@ -3,7 +3,7 @@ | |||
| use ddcommon::tag::Tag; | |||
| use serde::{Deserialize, Serialize}; | |||
|
|
|||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | |||
| #[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)] | |||
| pub struct CrashtrackerMetadata { | |||
There was a problem hiding this comment.
I disagree, in this case the default value is not valid and we should make it invalid by construction rather than validation.
For instance family really should be an enum, although right now it's more practical for it to be a String.
People should think about what they want in their metadata and deriving Default automatically goes against that
Comment on lines
+67
to
+70
| enum StringOrSerializedUri<'a> { | ||
| String(String), | ||
| SerializedUri(SerializedUri<'a>), | ||
| } |
There was a problem hiding this comment.
There is a reason why this is serialized like that and not just as a String (which it used to in the past).
The reason is that non http uris (unix sockets unix:// , windows pipes, or file uris) are not correctly deserialized by the hyper::Uri from string implementation.
Could you add unit tests to check what happens when you try to deserialize non http Uris?
I'm interested in particular in paths starting with /, ./ and containing spaces as they have caused problems
There was a problem hiding this comment.
Because this is passed to Hyper directly, I assumed it was just a shortcut to serialize Url which is not serializable by default. If there is a good reason to pass a more complex object, I'm fine with also passing that structure instead of relying on a string and removing this change. However, I tend to prefer handling this kind of common use cases internally behind the API instead of externally, thus exposing Hyper limitations to the caller.
There was a problem hiding this comment.
Actually, how do I even get the tests to pass? Whatever I do it seems there are a lot of tests that are failing before I even touch anything, and I'm unable to make them pass.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Improve crashtracker config deserialization:
Motivation
When using NAPI-RS, JavaScript types are converted directly to Rust types through serialization/deserialization. This meant that a lot of unneeded options were required to be passed, and the URL had to be passed with the internal structure as well.
Additional Notes
How to test the change?
Good question 😅 I'm not familiar with tests in libdatadog, and I don't know if serialization/deserialization is even tested at all right now.