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.
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
Implement Network Error Logging #2421
Implement Network Error Logging #2421
Changes from 51 commits
f863254
2f8a2d9
a695391
704592c
ae3c9cc
6fd93e0
2f376d2
a503eb3
78c2839
8029b3a
58ee827
0d2ffd9
b04fa10
1d8b7b6
0c7eda5
fec72d9
a8d7ee1
aa21e10
be0726c
71f844b
1c41705
1d072c2
c8cf938
7eaed71
90a8994
0686a91
d50c667
2297808
df467b8
efc6f64
ebf6ee9
925c42d
8a1f795
33e38bd
c92aefb
d894f2a
9ca755f
ed5b68f
be229ee
34f3aad
29a4cb6
5010cad
34014f0
f005272
e89bc63
eabb20b
a61d9a4
f2cdee0
bc3aafc
282ff20
2ea8209
ede374e
e860db5
d9de7f9
8530410
8b1de5a
c5777db
5858d8c
ddae542
82b8932
b1b3c68
13cc9e2
ecc9249
1d02da1
0c3773a
e6a4d27
2d4aba3
8bc4b23
61cd8c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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.
Same here wrt star exports.
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: If we make
type
an enum with another(String)
variant, we can probably shave off a string allocation for most use cases:https://developer.mozilla.org/en-US/docs/Web/HTTP/Network_Error_Logging#dns.unreachable
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.
pii = "maybe"
? If that is the target server IP, it is still sensitive information but does not identify users.IpAddr
type here?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.
done
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.
Could you please document, which unit this time is specified in?
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.
done
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.
Could we introduce an enumeration for the known phases, with an
Other(String)
variant as fallback? According to the specification, the known phases aredns
,connection
, andapplication
.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.
done
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.
Consider linking to the NEL context type for these docs, that way they don't have to be maintained in two places when we add more information to them.
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.
done
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 the
InvalidNel
error variant is not instantiated anywhere, and the only other variant covers deserialization. Could we simplify this by callingAnnotated::from_json_bytes(value).map_err(ProcessingError::InvalidJson)?
directly inevent_from_nel_item
like the other deserializing functions do?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.
done
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.
Is this still true?
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.
👍 nice catch, removed .
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.
Could we introduce an enum for the well-known protocols? Particularly, it would be nice to normalize the casing of the protocols, so that both
https
andHTTPS
can be sent in, but they are always stored in the same notation.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 could not find the list of the protocols to be used here. The doc is not really clear on this.
I can postpone this to later PRs