feat(metrics): Add received_at timestamp in bucket metadata #3488

iambriccardo · 2024-04-26T09:22:40Z

This PR adds a new received_at field to the BucketMetadata struct. The goal of this field is to eventually propagate it to Kafka to measure the end-to-end latency of our metrics pipeline. For this reason, the received_at measurement must happen in the outermost internal Relay.

The field received_at will be set/overridden by the outermost Relay via the option keep_metadata which when set to false will tell Relay to override the incoming metadata.

The merge operation on two received_at timestamps is defined as min(r1, r2) if both timestamps are non-null, otherwise the first non-null value will be taken. If both values are null, the merge result will still yield null.

Closes: https://github.com/getsentry/team-ingest/issues/265

iambriccardo · 2024-04-29T07:59:05Z

relay-config/src/config.rs

 }

 impl Default for SentryMetrics {
    fn default() -> Self {
        Self {
            meta_locations_expiry: 15 * 24 * 60 * 60,
            meta_locations_max: 5,
+            override_received_at_metadata: false,


By default we do not want to override the metadata to avoid unexpected behaviors due to the missing config param.

iambriccardo · 2024-04-29T07:59:39Z

relay-config/src/config.rs

@@ -524,13 +524,17 @@ struct SentryMetrics {
    ///
    /// Defaults to 5.
    pub meta_locations_max: usize,
+    /// Whether to override the [`received_at`] field in the [`BucketMetadata`] with the current
+    /// receive time of the instance.
+    pub override_received_at_metadata: bool,


In production, we would like to set this option to true only for pop relays, since we want to have the received_at metadata field set to the outermost relay timestamp that receives a given bucket.

We already have that handling in the processor as keep_metadata for example in handle_process_metrics which should already do the right thing.

relay-server/src/services/store.rs

iambriccardo · 2024-04-30T08:35:59Z

relay-server/src/metrics_extraction/generic.rs

@@ -56,13 +56,20 @@ where
            continue;
        };

+        // For extracted metrics we assume the `received_at` timestamp is equivalent to the time
+        // in which the metric is extracted.
+        #[cfg(not(test))]


Instead of mocking time, we generate it differently if we are in test, in order to make the unit tests deterministic.

iambriccardo · 2024-04-30T08:37:22Z

relay-server/src/services/processor.rs

+                bucket.metadata = BucketMetadata::default();
+            }
+
+            if override_received_at_metadata {


We purposefully override the metadata after the decision to keep it has been made.

This piece of code could be optimized to avoid overwriting a field but I felt it's more comprehensible like this.

iambriccardo · 2024-04-30T08:38:23Z

relay-server/src/services/processor.rs

+    }
+
+    #[tokio::test]
+    async fn test_process_batched_metrics_bucket_metadata() {


Will add integration tests in a follow up PR where we are also going to write the data to kafka.

Dav1dde · 2024-05-02T07:15:43Z

relay-config/src/config.rs

@@ -524,13 +524,17 @@ struct SentryMetrics {
    ///
    /// Defaults to 5.
    pub meta_locations_max: usize,
+    /// Whether to override the [`received_at`] field in the [`BucketMetadata`] with the current
+    /// receive time of the instance.
+    pub override_received_at_metadata: bool,


We already have that handling in the processor as keep_metadata for example in handle_process_metrics which should already do the right thing.

relay-metrics/src/bucket.rs

relay-server/src/metrics_extraction/generic.rs

relay-metrics/src/bucket.rs

relay-server/src/metrics_extraction/sessions/types.rs

relay-server/src/services/processor.rs

relay-server/src/services/store.rs

relay-server/src/services/processor.rs

Dav1dde · 2024-05-02T13:38:55Z

relay-metrics/src/bucket.rs

+        let mut buckets = serde_json::from_str::<Vec<Bucket>>(json).unwrap();
+        buckets[0].metadata = BucketMetadata::new(UnixTimestamp::from_secs(1615889440));


You can just put this into the json, right?

relay-server/src/services/processor.rs

iambriccardo · 2024-05-03T08:29:55Z

tests/integration/test_metrics.py

-            span_time = 9.910107
-        else:
-            span_time = 9.910106
+        span_time = 9.910106


Tests seem to succeed with the actual value 9.910106.

iambriccardo · 2024-05-03T08:30:08Z

tests/integration/test_metrics.py

        metrics[metric["name"]] = metric
        metrics["headers"][metric["name"]] = metric_headers

    metrics_consumer.assert_empty()
    return metrics


+def metrics_without_keys(received_metrics, keys):


Decided to opt for a generic solution that can remove any key.

Dav1dde · 2024-05-03T12:26:00Z

relay-server/src/testutils.rs

@@ -155,6 +155,34 @@ pub fn create_test_processor(config: Config) -> EnvelopeProcessorService {
    )
 }

+pub fn create_test_processor_with_addrs(


create_test_processor should forward to this

iambriccardo added 3 commits April 25, 2024 16:07

feat(metrics): Add received_at timestamp in bucket metadata

f8ab10c

Fix

9d51e9f

Fix

85c7dcb

iambriccardo changed the title ~~riccardo/feat/add timestamp~~ feat(metrics): Add received_at timestamp in bucket metadata Apr 26, 2024

iambriccardo added 6 commits April 26, 2024 12:50

Fix

d07aa34

Fix

9966952

Add logic

d341e53

Refactor tests

5649429

Fix

b520960

Fix

adab977

iambriccardo commented Apr 29, 2024

View reviewed changes

Fix

8ee95c5

iambriccardo commented Apr 29, 2024

View reviewed changes

relay-server/src/services/store.rs Show resolved Hide resolved

iambriccardo added 5 commits April 29, 2024 11:26

Merge branch 'master' into riccardo/feat/add-timestamp

640a26d

Add test

0e61ac1

Fix

a7d50f0

Fix

7766480

Revert

20bd2eb

iambriccardo commented Apr 30, 2024

View reviewed changes

iambriccardo added 5 commits April 30, 2024 10:43

Fix

3a12653

Fix

35a69cc

Update changelog

4d51d41

Merge

f80c03f

Fix

072f96e

iambriccardo marked this pull request as ready for review April 30, 2024 10:42

iambriccardo requested a review from a team as a code owner April 30, 2024 10:42

Dav1dde reviewed May 2, 2024

View reviewed changes

Improv

9519e31

iambriccardo requested a review from Dav1dde May 2, 2024 13:29

Remove

8ebdd62

Dav1dde reviewed May 2, 2024

View reviewed changes

iambriccardo added 3 commits May 2, 2024 15:59

Merge

d0d1b1e

Cleanup

b83fa01

Fix tests

24fc67f

iambriccardo commented May 3, 2024

View reviewed changes

iambriccardo and others added 7 commits May 3, 2024 10:30

Rename

5c5680b

Rename

768eae6

Rename

f9bdf3f

Remove

cc1b985

Add back snapshots

22443d0

Merge branch 'master' into riccardo/feat/add-timestamp

c4b0b13

fix: More snapshots

0244cb6

iambriccardo requested a review from Dav1dde May 3, 2024 11:57

Dav1dde approved these changes May 3, 2024

View reviewed changes

iambriccardo merged commit f5abf09 into master May 6, 2024
28 checks passed

iambriccardo deleted the riccardo/feat/add-timestamp branch May 6, 2024 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metrics): Add received_at timestamp in bucket metadata #3488

feat(metrics): Add received_at timestamp in bucket metadata #3488

iambriccardo commented Apr 26, 2024 •

edited

Loading

iambriccardo Apr 29, 2024

iambriccardo Apr 29, 2024

Dav1dde May 2, 2024

iambriccardo Apr 30, 2024

iambriccardo Apr 30, 2024

iambriccardo Apr 30, 2024

iambriccardo Apr 30, 2024

Dav1dde May 2, 2024

Dav1dde May 2, 2024

iambriccardo May 3, 2024

iambriccardo May 3, 2024

Dav1dde May 3, 2024

		let mut buckets = serde_json::from_str::<Vec<Bucket>>(json).unwrap();
		buckets[0].metadata = BucketMetadata::new(UnixTimestamp::from_secs(1615889440));

feat(metrics): Add received_at timestamp in bucket metadata #3488

feat(metrics): Add received_at timestamp in bucket metadata #3488

Conversation

iambriccardo commented Apr 26, 2024 • edited Loading

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

iambriccardo commented Apr 26, 2024 •

edited

Loading