Merge branch 'master' into build/jemalloc

* master: ci(gha): add debugging for "Couldn'\''t write tracker file" issue (#2081) feat(crons): Pass-through minimal `sdk` field (#2073) fix(quota): Parse large limits (#2079) instr(server): More details on unexpected envelope drop (#2077) fix(ci): Skip flaky integration test (#2076)
getsentry · May 2, 2023 · cb3dfb6 · cb3dfb6
2 parents 3063dee + dda830c
commit cb3dfb6
Show file tree

Hide file tree

Showing 7 changed files with 127 additions and 6 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -320,15 +320,26 @@ jobs:
       - name: Upload gocd deployment assets
         run: |
           set -euxo pipefail
-          VERSION="$(docker run --rm "$IMG_VERSIONED" --version | cut -d' ' -f2)"
+          VERSION="$(docker run --rm "$IMG_VERSIONED" --version | cut -d" " -f2)"
           echo "relay@$VERSION+$REVISION" > release-name
 
           docker run --rm --entrypoint cat "$IMG_VERSIONED" /opt/relay-debug.zip > relay-debug.zip
           docker run --rm --entrypoint cat "$IMG_VERSIONED" /opt/relay.src.zip > relay.src.zip
           docker run --rm --entrypoint tar "$IMG_VERSIONED" -cf - /lib/x86_64-linux-gnu > libs.tar
 
-          gsutil -m cp ./release-name ./libs.tar ./relay-debug.zip ./relay.src.zip \
-          "gs://dicd-team-devinfra-cd--relay/deployment-assets/$REVISION/"
+          # debugging for mysterious "Couldn't write tracker file" issue:
+          (env | grep runner) || true
+          ls -ld  \
+            /home \
+            /home/runner \
+            /home/runner/.gsutil \
+            /home/runner/.gsutil/tracker-files \
+            /home/runner/.gsutil/tracker-files/upload_TRACKER_*.rc.zip__JSON.url \
+          || true
+          gsutil -m cp -L gsutil.log ./libs.tar ./relay-debug.zip ./relay.src.zip ./release-name \
+            "gs://dicd-team-devinfra-cd--relay/deployment-assets/$REVISION/" || status=$? && status=$?
+          cat gsutil.log
+          exit "$status"
 
   test_integration:
     name: Integration Tests

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 **Bug Fixes**:
 
 - Enforce rate limits for monitor check-ins. ([#2065](https://github.com/getsentry/relay/pull/2065))
+- Allow rate limits greater than `u32::MAX`. ([#2079](https://github.com/getsentry/relay/pull/2079))
 
 **Features**:
 

diff --git a/relay-monitors/src/lib.rs b/relay-monitors/src/lib.rs
@@ -100,6 +100,17 @@ struct MonitorConfig {
     timezone: Option<String>,
 }
 
+// XXX(epurkhiser): This is a duplicate of the ClientSdkInfo that is part of the relay-general
+// crate. Until we're able to migrate the checkin payload over to it's own protocol using
+// metastructure we'll need to have this duplicated here
+//
+/// The SDK Interface describes the Sentry SDK and its configuration used to capture and transmit an event.
+#[derive(Debug, Deserialize, Serialize)]
+struct MinimalClientSdkInfo {
+    pub name: String,
+    pub version: String,
+}
+
 /// The monitor check-in payload.
 #[derive(Debug, Deserialize, Serialize)]
 struct CheckIn {
@@ -113,6 +124,10 @@ struct CheckIn {
     /// Status of this check-in. Defaults to `"unknown"`.
     status: CheckInStatus,
 
+    /// monitor configuration to support upserts.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    sdk: Option<MinimalClientSdkInfo>,
+
     /// The environment to associate the check-in with
     #[serde(default, skip_serializing_if = "Option::is_none")]
     environment: Option<String>,
@@ -176,6 +191,10 @@ mod tests {
   "check_in_id": "a460c25ff2554577b920fcfacae4e5eb",
   "monitor_slug": "my-monitor",
   "status": "in_progress",
+  "sdk": {
+    "name": "sentry.rust",
+    "version": "1.0.0"
+  },
   "environment": "production",
   "duration": 21.0
 }"#;
@@ -192,6 +211,10 @@ mod tests {
   "check_in_id": "a460c25ff2554577b920fcfacae4e5eb",
   "monitor_slug": "my-monitor",
   "status": "in_progress",
+  "sdk": {
+    "name": "sentry.rust",
+    "version": "1.0.0"
+  },
   "monitor_config": {
     "schedule": {
       "type": "crontab",
@@ -212,6 +235,10 @@ mod tests {
   "check_in_id": "a460c25ff2554577b920fcfacae4e5eb",
   "monitor_slug": "my-monitor",
   "status": "in_progress",
+  "sdk": {
+    "name": "sentry.rust",
+    "version": "1.0.0"
+  },
   "monitor_config": {
     "schedule": {
       "type": "interval",
@@ -236,6 +263,10 @@ mod tests {
   "check_in_id": "a460c25ff2554577b920fcfacae4e5eb",
   "monitor_slug": "my-monitor",
   "status": "in_progress",
+  "sdk": {
+    "name": "sentry.rust",
+    "version": "1.0.0"
+  },
   "monitor_config": {
     "schedule": {
       "type": "crontab",

diff --git a/relay-quotas/src/quota.rs b/relay-quotas/src/quota.rs
@@ -244,8 +244,10 @@ pub struct Quota {
     /// Maximum number of matching events allowed. Can be `0` to reject all events, `None` for an
     /// unlimited counted quota, or a positive number for enforcement. Requires `window` if the
     /// limit is not `0`.
+    ///
+    /// For attachments, this limit expresses the number of allowed bytes.
     #[serde(default)]
-    pub limit: Option<u32>,
+    pub limit: Option<u64>,
 
     /// The time window in seconds to enforce this quota in. Required in all cases except `limit=0`,
     /// since those quotas are not measured.
@@ -409,6 +411,32 @@ mod tests {
         "###);
     }
 
+    #[test]
+    fn test_parse_quota_project_large() {
+        let json = r#"{
+            "id": "p",
+            "scope": "project",
+            "scopeId": "1",
+            "limit": 4294967296,
+            "window": 42,
+            "reasonCode": "not_so_fast"
+        }"#;
+
+        let quota = serde_json::from_str::<Quota>(json).expect("parse quota");
+
+        insta::assert_ron_snapshot!(quota, @r###"
+        Quota(
+          id: Some("p"),
+          categories: [],
+          scope: project,
+          scopeId: Some("1"),
+          limit: Some(4294967296),
+          window: Some(42),
+          reasonCode: Some(ReasonCode("not_so_fast")),
+        )
+        "###);
+    }
+
     #[test]
     fn test_parse_quota_key() {
         let json = r#"{

diff --git a/relay-quotas/src/redis.rs b/relay-quotas/src/redis.rs
@@ -79,7 +79,10 @@ impl<'a> RedisQuota<'a> {
 
     /// Returns the limit value for Redis (`-1` for unlimited, otherwise the limit value).
     fn limit(&self) -> i64 {
-        self.limit.map(i64::from).unwrap_or(-1)
+        self.limit
+            // If it does not fit into i64, treat as unlimited:
+            .and_then(|limit| limit.try_into().ok())
+            .unwrap_or(-1)
     }
 
     fn shift(&self) -> u64 {
@@ -655,6 +658,33 @@ mod tests {
         assert_eq!(redis_quota.key(), "quota:foo{69420}:23453");
     }
 
+    #[test]
+    fn test_large_redis_limit_large() {
+        let quota = Quota {
+            id: Some("foo".to_owned()),
+            categories: DataCategories::new(),
+            scope: QuotaScope::Organization,
+            scope_id: None,
+            window: Some(10),
+            limit: Some(9223372036854775808), // i64::MAX + 1
+            reason_code: None,
+        };
+
+        let scoping = ItemScoping {
+            category: DataCategory::Error,
+            scoping: &Scoping {
+                organization_id: 69420,
+                project_id: ProjectId::new(42),
+                project_key: ProjectKey::parse("a94ae32be2584e0bbd7a4cbb95971fee").unwrap(),
+                key_id: Some(4711),
+            },
+        };
+
+        let timestamp = UnixTimestamp::from_secs(234_531);
+        let redis_quota = RedisQuota::new(&quota, scoping, timestamp).unwrap();
+        assert_eq!(redis_quota.limit(), -1);
+    }
+
     #[test]
     #[allow(clippy::disallowed_names, clippy::let_unit_value)]
     fn test_is_rate_limited_script() {

diff --git a/relay-server/src/utils/managed_envelope.rs b/relay-server/src/utils/managed_envelope.rs
@@ -290,7 +290,26 @@ impl ManagedEnvelope {
         let handling = Handling::from_outcome(&outcome);
         match handling {
             Handling::Success => relay_log::debug!("dropped envelope: {}", outcome),
-            Handling::Failure => relay_log::error!("dropped envelope: {}", outcome),
+            Handling::Failure => {
+                let summary = &self.context.summary;
+                relay_log::with_scope(
+                    |scope| {
+                        scope.set_tag("event_category", format!("{:?}", summary.event_category));
+                        scope.set_tag("has_attachments", summary.attachment_quantity > 0);
+                        scope.set_tag("has_sessions", summary.session_quantity > 0);
+                        scope.set_tag("has_profiles", summary.profile_quantity > 0);
+                        scope.set_tag("has_replays", summary.replay_quantity > 0);
+                        scope.set_tag("has_checkins", summary.checkin_quantity > 0);
+                        scope.set_tag("event_category", format!("{:?}", summary.event_category));
+                        scope.set_extra("cached_summary", format!("{:?}", summary).into());
+                        scope.set_extra(
+                            "recomputed_summary",
+                            format!("{:?}", EnvelopeSummary::compute(self.envelope())).into(),
+                        );
+                    },
+                    || relay_log::error!("dropped envelope: {}", outcome),
+                );
+            }
         }
 
         // TODO: This could be optimized with Capture::should_capture

diff --git a/tests/integration/test_metrics.py b/tests/integration/test_metrics.py
@@ -960,6 +960,7 @@ def test_no_transaction_metrics_when_filtered(mini_sentry, relay):
     assert mini_sentry.captured_events.qsize() == 0
 
 
+@pytest.mark.skip(reason="flake")
 def test_graceful_shutdown(mini_sentry, relay):
     relay = relay(
         mini_sentry,