Implement Network Error Logging #2421

oioki · 2023-08-21T14:09:32Z

The first part of the NEL (Network Error Logging) implementation.

Adds a new Nel EventType
Adds a new Nel protocol with minimal validation (i.e. only specific fields are captured)
Adds a new /api/:project_id/nel/ endpoint for NEL reports ingestion
- simple content type validation
- splits payload into separate envelopes (a single HTTP NEL request could contain several independent reports)
user.ip_address field is set to the IP address of the request (real user's IP address)
An event is enriched with browser information derived from the request's User-Agent header

Related PRs:
getsentry/sentry#55135
getsentry/sentry-kafka-schemas#177

jjbayer

LGTM. Do nel reports respect rate limits for errors now? Can we test that? See https://github.com/getsentry/relay/pull/2421/files#r1360800171.

Also, this convo is still open, but feel free to ignore if you think it's not worth it.

jjbayer · 2023-10-17T09:30:50Z

py/CHANGELOG.md

@@ -3,6 +3,7 @@
 ## Unreleased

 - Add `scraping_attempts` field to the event schema. ([#2575](https://github.com/getsentry/relay/pull/2575))
+- Added support for NEL (Network Error Logging) reports ingestion. ([#2421](https://github.com/getsentry/relay/pull/2421))


nit

Suggested change

- Added support for NEL (Network Error Logging) reports ingestion. ([#2421](https://github.com/getsentry/relay/pull/2421))

- Add context for NEL (Network Error Logging) reports to the event schema. ([#2421](https://github.com/getsentry/relay/pull/2421))

jjbayer · 2023-10-17T09:32:18Z

relay-event-schema/src/protocol/nel.rs

+/// NOTE: This is the structure used inside the Event (serialization is based on Annotated
+/// infrastructure). We also use a version of this structure to deserialize from raw JSON
+/// via serde.


Is this still true?

👍 nice catch, removed .

jjbayer · 2023-10-17T09:39:58Z

relay-server/src/envelope.rs

    where
+        C: Fn(usize) -> bool,


Sorry, I missed that the condition for returning None is different for the two functions., this makes the helper function less helpful than I thought. Does the condition have to be different though?

yeah, the condition are different and must stay this way

olksdr · 2023-10-17T13:37:42Z

LGTM. Do nel reports respect rate limits for errors now? Can we test that? See https://github.com/getsentry/relay/pull/2421/files#r1360800171.

Also, this convo is still open, but feel free to ignore if you think it's not worth it.

@jjbayer yeah. I will still take time and look into adding a test.

iker-barriocanal

Really good stuff 🚀

iker-barriocanal · 2023-10-18T12:46:10Z

relay-server/src/utils/mod.rs

@@ -27,6 +28,7 @@ pub use self::metrics_rate_limits::*;
 pub use self::multipart::*;
 #[cfg(feature = "processing")]
 pub use self::native::*;
+pub use self::nel::*;


Should we just export the items we need?

iker-barriocanal · 2023-10-18T12:58:58Z

relay-event-schema/src/protocol/contexts/mod.rs

@@ -19,6 +20,7 @@ pub use cloud_resource::*;
 pub use device::*;
 pub use gpu::*;
 pub use monitor::*;
+pub use nel::*;


Same here wrt star exports.

jan-auer

Thank you, this looks great and is in shippable state.

See a number of smaller comments attached, and feel free to merge without another review from me.

relay-event-schema/src/protocol/contexts/nel.rs

jan-auer · 2023-10-30T07:44:53Z

relay-event-schema/src/protocol/contexts/nel.rs

+    /// If request failed, the type of its network error. If request succeeded, "ok".
+    pub error_type: Annotated<String>,
+    /// Server IP where the requests was sent to.
+    #[metastructure(pii = "true")]


Should we rather mark that as pii = "maybe"? If that is the target server IP, it is still sensitive information but does not identify users.

Could we use the IpAddr type here?

jan-auer · 2023-10-30T07:47:57Z

relay-event-schema/src/protocol/contexts/nel.rs

+    /// The time between the start of the resource fetch and when it was completed or aborted.
+    pub elapsed_time: Annotated<u64>,
+    /// If request failed, the phase of its network error. If request succeeded, "application".
+    pub phase: Annotated<String>,


Could we introduce an enumeration for the known phases, with an Other(String) variant as fallback? According to the specification, the known phases are dns, connection, and application.

jan-auer · 2023-10-30T07:48:27Z

relay-event-schema/src/protocol/contexts/nel.rs

+    /// Server IP where the requests was sent to.
+    #[metastructure(pii = "true")]
+    pub server_ip: Annotated<String>,
+    /// The time between the start of the resource fetch and when it was completed or aborted.


Could you please document, which unit this time is specified in?

jan-auer · 2023-10-30T07:52:41Z

relay-event-schema/src/protocol/nel.rs

+//!
+//! NEL is a browser feature that allows reporting of failed network requests from the client side.
+//! W3C Editor's Draft: <https://w3c.github.io/network-error-logging/>
+//! MDN: <https://developer.mozilla.org/en-US/docs/Web/HTTP/Network_Error_Logging>


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.

jan-auer · 2023-10-30T07:57:13Z

relay-event-schema/src/protocol/nel.rs

+    #[error("unexpected format")]
+    InvalidNel,


It looks like the InvalidNel error variant is not instantiated anywhere, and the only other variant covers deserialization. Could we simplify this by calling Annotated::from_json_bytes(value).map_err(ProcessingError::InvalidJson)? directly in event_from_nel_item like the other deserializing functions do?

jan-auer · 2023-10-30T07:58:30Z

relay-event-schema/src/protocol/request.rs

@@ -444,6 +444,9 @@ pub struct Request {
    /// HTTP request method.
    pub method: Annotated<String>,

+    /// HTTP protocol.
+    pub protocol: Annotated<String>,


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 and HTTPS can be sent in, but they are always stored in the same notation.

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

jan-auer · 2023-10-30T08:07:31Z

relay-server/src/endpoints/nel.rs

+        return Ok(StatusCode::UNSUPPORTED_MEDIA_TYPE.into_response());
+    }
+
+    let items: Vec<&RawValue> =


In practice, allocating a vector here should be fine. However, since there's currently no limit placed on the number of NEL reports in an envelope - neither in the endpoint nor where it gets sent into validation (common.rs) -- let's please be sure we have enough visibility into this endpoint to add more safety measures in the future.

If this turns out to be a problem, we can easily make this a streaming deserializer.

jan-auer · 2023-10-30T08:10:42Z

relay-server/src/utils/nel.rs

+use relay_protocol::Annotated;
+
+/// Enriches the event with new values using the provided [`NetworkReportRaw`].
+pub fn enrich_nel_event(event: &mut Event, nel: Annotated<NetworkReportRaw>) {


Could this function be moved into relay-normalize?

Either way works, as there's currently no clear criteria for where normalization is called and where the code is located. Particularly, this is not part of the current "store normalizer" but rather called during deserialization like for security reports.

However, in an effort to reduce the amount of schema-specific business logic that resides in relay-server, a better place for these sorts of functions would be the normalize crate.

Yeah, I can move it to normalize. It was initially there, but then was moved back to the server utils.

jan-auer · 2023-10-30T08:23:50Z

relay-server/src/utils/nel.rs

+    }
+
+    // Set the timestamp on the event when it actually occurred.
+    let now: DateTime<Utc> = Utc::now();


Could we use the receive timestamp from the envelope instead of Utc::now()? Relay might have delayed processing the item for various reasons, but the receive timestamp is guaranteed to be as accurate as possible.

Co-authored-by: Jan Michael Auer <mail@jauer.org>

Sentry backend part of the NEL ([Network Error Logging](https://developer.mozilla.org/en-US/docs/Web/HTTP/Network_Error_Logging)) implementation. * Added a new `nel` event type and interface * Special logic for metadata and culprit generation To be deployed after merging Relay part (getsentry/relay#2421) and releasing `sentry-relay` python package. --------- Co-authored-by: mdtro <matthew.trostel@sentry.io> Co-authored-by: Joris Bayer <joris.bayer@sentry.io>

oioki and others added 6 commits August 21, 2023 10:51

hack nel

f863254

Fix Nel endpoint

2f8a2d9

moar hacking

a695391

cleanup

704592c

cleanup warnings

ae3c9cc

tiny cleanup

6fd93e0

oioki requested review from olksdr and mdtro August 21, 2023 14:09

oioki and others added 10 commits August 21, 2023 21:00

add browser contexts from user-agent header

2f376d2

add logger and placeholder for culprit

a503eb3

Add tags, user and make sure we have "type" in the type

78c2839

fix tests

8029b3a

Fix lint

58ee827

Wrong user ip

0d2ffd9

Normalize NEL

b04fa10

Change the timestamp of the event

1d8b7b6

removed culprit (it will be set on sentry side)

0c7eda5

split issues by HTTP status code

fec72d9

jan-auer changed the title ~~Hackweek: NEL~~ hackweek: NEL Aug 25, 2023

oioki and others added 11 commits September 26, 2023 15:20

merge master; fix conflics

a8d7ee1

fixes

aa21e10

emit envelope for each NEL report item

be0726c

Merge branch 'master' into hackweek/nel

71f844b

remove unused vars

1c41705

Merge branch 'master' into hackweek/nel

1d072c2

Hide JsonSchema behind the feature flag

c8cf938

add Nel event type

7eaed71

update test snapshot

90a8994

add test for test_nel_basic

0686a91

bit more docs, fix typo

d50c667

olksdr added 3 commits October 17, 2023 06:52

Merge remote-tracking branch 'origin/master' into hackweek/nel

bc3aafc

Address review comments

282ff20

refactor repetitions in the code

2ea8209

jjbayer approved these changes Oct 17, 2023

View reviewed changes

olksdr added 2 commits October 17, 2023 15:35

PR review. Fix typos

ede374e

Remove wrong doc comment

e860db5

olksdr added 4 commits October 17, 2023 15:50

Explicitly set the event id for the new item

d9de7f9

Make sure that each item has differnet event id in the envelope

8530410

set status code conditionaly, remove dbg

8b1de5a

Merge branch 'master' into hackweek/nel

c5777db

iker-barriocanal approved these changes Oct 18, 2023

View reviewed changes

olksdr added 2 commits October 19, 2023 08:23

Merge branch 'master' into hackweek/nel

5858d8c

Update test "test_processing_quotas" to include "nel" quota checks

ddae542

jan-auer approved these changes Oct 30, 2023

View reviewed changes

jan-auer reviewed Oct 30, 2023

View reviewed changes

oioki and others added 10 commits October 30, 2023 15:27

Update relay-event-schema/src/protocol/contexts/nel.rs

82b8932

Co-authored-by: Jan Michael Auer <mail@jauer.org>

Merge branch 'master' into hackweek/nel

b1b3c68

review comments

13cc9e2

fix lint and tests

ecc9249

Merge branch 'master' into hackweek/nel

1d02da1

Move functionality to normalization crate

0c3773a

Bump timeout

e6a4d27

Merge remote-tracking branch 'origin/master' into hackweek/nel

2d4aba3

Merge branch 'master' into hackweek/nel

8bc4b23

Use event timestamp when available for calculation

61cd8c9

oioki merged commit de5e0fa into master Nov 6, 2023
20 checks passed

oioki deleted the hackweek/nel branch November 6, 2023 12:02

olksdr mentioned this pull request Mar 27, 2024

Add support for different types of reports coming from Reporting API #3346

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Network Error Logging #2421

Implement Network Error Logging #2421

oioki commented Aug 21, 2023 •

edited

Loading

jjbayer left a comment

jjbayer Oct 17, 2023

jjbayer Oct 17, 2023

olksdr Oct 17, 2023

jjbayer Oct 17, 2023

olksdr Oct 17, 2023

olksdr commented Oct 17, 2023

iker-barriocanal left a comment

iker-barriocanal Oct 18, 2023

iker-barriocanal Oct 18, 2023

jan-auer left a comment

jan-auer Oct 30, 2023

olksdr Oct 31, 2023

jan-auer Oct 30, 2023

olksdr Oct 31, 2023

jan-auer Oct 30, 2023

olksdr Oct 31, 2023

jan-auer Oct 30, 2023

olksdr Oct 31, 2023

jan-auer Oct 30, 2023

olksdr Oct 31, 2023

jan-auer Oct 30, 2023

olksdr Oct 31, 2023

jan-auer Oct 30, 2023

jan-auer Oct 30, 2023

olksdr Oct 31, 2023

jan-auer Oct 30, 2023

	- Added support for NEL (Network Error Logging) reports ingestion. ([#2421](https://github.com/getsentry/relay/pull/2421))
	- Add context for NEL (Network Error Logging) reports to the event schema. ([#2421](https://github.com/getsentry/relay/pull/2421))

Implement Network Error Logging #2421

Implement Network Error Logging #2421

Conversation

oioki commented Aug 21, 2023 • edited Loading

jjbayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olksdr commented Oct 17, 2023

iker-barriocanal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oioki commented Aug 21, 2023 •

edited

Loading