Skip to content

Commit

Permalink
ref(server): Remove noisy payload parsing errors (#1032)
Browse files Browse the repository at this point in the history
Removes errors when event payloads or session payloads cannot be parsed.
These were reported as "failed to extract event" and "failed to store
session", respectively.

Such errors are completely expected when invalid payloads are sent into
Relay. They were originally introduced to debug broken SDKs, but in the
way they are being reported they are neither actionable to the Relay
development team, nor anyone running Relay onpremise.

Eventually, we will bring this back in form of a metric including the
SDK version, and potentially a mechanism to sample invalid payloads. For
now, these errors are not actionable, and drown real issues in Sentry.
  • Loading branch information
jan-auer committed Jul 2, 2021
1 parent abff616 commit 49d6313
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 30 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
**Bug Fixes**:

- Remove connection metrics reported under `connector.*`. They have been fully disabled since version `21.3.0`. ([#1021](https://github.com/getsentry/relay/pull/1021))
- Remove error logs for "failed to extract event" and "failed to store session". ([#1032](https://github.com/getsentry/relay/pull/1032))

**Internal**:

Expand Down
22 changes: 8 additions & 14 deletions relay-server/src/actors/envelopes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -521,14 +521,8 @@ impl EnvelopeProcessor {
let mut session = match SessionUpdate::parse(&payload) {
Ok(session) => session,
Err(error) => {
return relay_log::with_scope(
|s| s.set_extra("session", String::from_utf8_lossy(&payload).into()),
|| {
// Skip gracefully here to allow sending other sessions.
relay_log::error!("failed to store session: {}", LogError(&error));
false
},
);
relay_log::trace!("skipping invalid session payload: {}", LogError(&error));
return false;
}
};

Expand Down Expand Up @@ -749,7 +743,7 @@ impl EnvelopeProcessor {
};

if let Err(json_error) = apply_result {
// logged at call site of extract_event
// logged in extract_event
relay_log::configure_scope(|scope| {
scope.set_extra("payload", String::from_utf8_lossy(&data).into());
});
Expand Down Expand Up @@ -997,7 +991,10 @@ impl EnvelopeProcessor {
} else if let Some(mut item) = raw_security_item {
relay_log::trace!("processing security report");
state.sample_rates = item.take_sample_rates();
self.event_from_security_report(item)?
self.event_from_security_report(item).map_err(|error| {
relay_log::error!("failed to extract security report: {}", LogError(&error));
error
})?
} else if attachment_item.is_some() || breadcrumbs1.is_some() || breadcrumbs2.is_some() {
relay_log::trace!("extracting attached event data");
Self::event_from_attachments(&self.config, attachment_item, breadcrumbs1, breadcrumbs2)?
Expand Down Expand Up @@ -1416,10 +1413,7 @@ impl EnvelopeProcessor {
self.expand_unreal(&mut state)?;
});

self.extract_event(&mut state).map_err(|error| {
relay_log::error!("failed to extract event: {}", LogError(&error));
error
})?;
self.extract_event(&mut state)?;

if_processing!({
self.process_unreal(&mut state)?;
Expand Down
8 changes: 6 additions & 2 deletions relay-server/src/actors/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,14 @@ use failure::{Fail, ResultExt};
use rdkafka::error::KafkaError;
use rdkafka::producer::BaseRecord;
use rdkafka::ClientConfig;
use relay_metrics::{Bucket, BucketValue, MetricUnit};
use rmp_serde::encode::Error as RmpError;
use serde::{ser::Error, Serialize};

use relay_common::{metric, ProjectId, UnixTimestamp, Uuid};
use relay_config::{Config, KafkaTopic};
use relay_general::protocol::{self, EventId, SessionAggregates, SessionStatus, SessionUpdate};
use relay_log::LogError;
use relay_metrics::{Bucket, BucketValue, MetricUnit};
use relay_quotas::Scoping;

use crate::envelope::{AttachmentType, Envelope, Item, ItemType};
Expand Down Expand Up @@ -170,7 +171,10 @@ impl StoreForwarder {
ItemType::Session => {
let mut session = match SessionUpdate::parse(&item.payload()) {
Ok(session) => session,
Err(_) => return Ok(()),
Err(error) => {
relay_log::error!("failed to store session: {}", LogError(&error));
return Ok(());
}
};

if session.status == SessionStatus::Errored {
Expand Down
24 changes: 10 additions & 14 deletions tests/integration/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -355,20 +355,16 @@ def test_session_release_required(
timestamp = datetime.now(tz=timezone.utc)
started = timestamp - timedelta(days=5, hours=1)

try:
relay.send_session(
project_id,
{
"sid": "8333339f-5675-4f89-a9a0-1c935255ab58",
"timestamp": timestamp.isoformat(),
"started": started.isoformat(),
},
)

sessions_consumer.assert_empty()
assert mini_sentry.test_failures
finally:
mini_sentry.test_failures.clear()
relay.send_session(
project_id,
{
"sid": "8333339f-5675-4f89-a9a0-1c935255ab58",
"timestamp": timestamp.isoformat(),
"started": started.isoformat(),
},
)

sessions_consumer.assert_empty()


def test_session_quotas(mini_sentry, relay_with_processing, sessions_consumer):
Expand Down

0 comments on commit 49d6313

Please sign in to comment.