ref: Kafka topic configuration #3282

lynnagara · 2024-03-18T18:50:17Z

As part of the kafka control plane project, Kafka config for all Sentry services should be automatically generated from a single source of truth in the ops/sentry-kafka-schemas repos.

The system knows about the default topic name + any override value per environment if relevant, but is not aware of the Relay specific values like "events" and "metrics".

This is aligned with the configuration format of Sentry, Snuba and super-big-consumers.

As part of the kafka control plane project, Kafka config for all Sentry services should be automatically generated from a single source of truth in ops/sentry-kafka-schemas. The system knows about the default topic name + override value per environment, but is not aware of the Relay specific values like "events" and "metrics". This is aligned with the configuration format of Sentry, Snuba and super-big-consumers.

lynnagara · 2024-03-18T18:50:48Z

relay-kafka/src/config.rs

 pub struct TopicAssignments {
    /// Simple events topic name.
+    #[serde(alias = "ingest-events")]


alias is supposed to be temporary, while we cut over config in ops. Later this should change to rename

lynnagara · 2024-03-18T18:52:14Z

relay-kafka/src/config.rs

@@ -156,8 +167,7 @@ impl Default for TopicAssignments {
            outcomes: "outcomes".to_owned().into(),
            outcomes_billing: None,
            sessions: "ingest-sessions".to_owned().into(),
-            metrics: "ingest-metrics".to_owned().into(),


I removed this because I think it's confusing / error prone. There shouldn't be a metrics default. Only 2 completely separate topics for either release health metrics and generic metrics.

lynnagara · 2024-03-18T18:53:50Z

relay-kafka/src/config.rs

@@ -156,8 +167,7 @@ impl Default for TopicAssignments {
            outcomes: "outcomes".to_owned().into(),
            outcomes_billing: None,
            sessions: "ingest-sessions".to_owned().into(),
-            metrics: "ingest-metrics".to_owned().into(),
-            metrics_sessions: None,
+            metrics_sessions: "ingest-metrics".to_owned().into(),


Now there is duplication here between in default and the "alias" values, which I would like to avoid. Not sure the best way to tackle this.

I think that's fine.

relay-kafka/src/config.rs

Dav1dde · 2024-03-19T09:03:00Z

relay-kafka/src/config.rs

-    /// Topic name for metrics extracted from sessions. Defaults to the assignment of `metrics`.
-    pub metrics_sessions: Option<TopicAssignment>,
+    /// Topic name for metrics extracted from sessions, aka release health.
+    #[serde(alias = "metrics")]


This is tricky, if both metrics and ingest-metrics are defined the precedence isn't clear (it was before). That being sad, I do like the change.

This is intended to be very temporary just while we cut things over in production. After the migration I think we should be very strict and only support the actual topic names

I meant, that if someone currently has both topics configured the precedence has now changed, which is a breaking change and this might affect us as well.

I double checked ST and prod -- we don't use both in any of the environments we control at least.

Dav1dde · 2024-03-19T09:04:15Z

relay-kafka/src/config.rs

@@ -156,8 +167,7 @@ impl Default for TopicAssignments {
            outcomes: "outcomes".to_owned().into(),
            outcomes_billing: None,
            sessions: "ingest-sessions".to_owned().into(),
-            metrics: "ingest-metrics".to_owned().into(),
-            metrics_sessions: None,
+            metrics_sessions: "ingest-metrics".to_owned().into(),


I think that's fine.

relay-kafka/src/config.rs

Dav1dde · 2024-03-20T07:01:50Z

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Changelog

 ## Unreleased
+- Kafka topic config supports default topic names as keys ([#3282](https://github.com/getsentry/relay/pull/3282))


Suggested change

- Kafka topic config supports default topic names as keys ([#3282](https://github.com/getsentry/relay/pull/3282))

- Kafka topic config supports default topic names as keys. ([#3282](https://github.com/getsentry/relay/pull/3282))

Out of curiosity, is this normally generated by hand?

Yes, usually done by hand, but many PRs without impact have no changelog entry (this one should)

Dav1dde · 2024-03-20T07:05:39Z

relay-kafka/src/config.rs

-    /// Topic name for metrics extracted from sessions. Defaults to the assignment of `metrics`.
-    pub metrics_sessions: Option<TopicAssignment>,
+    /// Topic name for metrics extracted from sessions, aka release health.
+    #[serde(alias = "metrics")]


I meant, that if someone currently has both topics configured the precedence has now changed, which is a breaking change and this might affect us as well.

Dav1dde · 2024-03-20T07:07:52Z

relay-kafka/src/config.rs

    pub transactions: TopicAssignment,
    /// Outcomes topic name.
+    #[serde(alias = "outcomes")]


Field is already called outcomes.

Dav1dde · 2024-03-20T07:09:03Z

relay-kafka/src/config.rs

    #[serde(alias = "metrics_transactions")]
+    #[serde(alias = "ingest-generic-metrics")]


Suggested change

#[serde(alias = "metrics_transactions")]

#[serde(alias = "ingest-generic-metrics")]

#[serde(alias = "metrics_transactions", alias = "ingest-generic-metrics")]

Dav1dde · 2024-03-20T07:10:21Z

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Changelog

 ## Unreleased
+- Kafka topic config supports default topic names as keys ([#3282](https://github.com/getsentry/relay/pull/3282))


Please move it to the Internal section.

lynnagara requested a review from a team as a code owner March 18, 2024 18:50

lynnagara commented Mar 18, 2024

View reviewed changes

add metrics alias

0db7d1b

Dav1dde reviewed Mar 19, 2024

View reviewed changes

Dav1dde assigned lynnagara Mar 19, 2024

lynnagara added 5 commits March 19, 2024 13:13

changelog

d98acb5

remove deny_unknown_fields

399d78f

.

897c360

improve tests

8b7b3ce

Merge remote-tracking branch 'origin/master' into kafka-topic-config

fe939da

Dav1dde approved these changes Mar 20, 2024

View reviewed changes

lynnagara added 3 commits March 20, 2024 09:58

changelog, fixes

99e1b4f

update changelog again

bb3ce9f

alias

6b324ef

lynnagara merged commit 4cb86c9 into master Mar 20, 2024
20 checks passed

lynnagara deleted the kafka-topic-config branch March 20, 2024 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: Kafka topic configuration #3282

ref: Kafka topic configuration #3282

lynnagara commented Mar 18, 2024

lynnagara Mar 18, 2024

lynnagara Mar 18, 2024

lynnagara Mar 18, 2024

Dav1dde Mar 19, 2024

Dav1dde Mar 19, 2024

lynnagara Mar 19, 2024

Dav1dde Mar 20, 2024

lynnagara Mar 20, 2024

Dav1dde Mar 19, 2024

Dav1dde Mar 20, 2024

lynnagara Mar 20, 2024

Dav1dde Mar 20, 2024

Dav1dde Mar 20, 2024

Dav1dde Mar 20, 2024

Dav1dde Mar 20, 2024

Dav1dde Mar 20, 2024

	- Kafka topic config supports default topic names as keys ([#3282](https://github.com/getsentry/relay/pull/3282))
	- Kafka topic config supports default topic names as keys. ([#3282](https://github.com/getsentry/relay/pull/3282))

		#[serde(alias = "metrics_transactions")]
		#[serde(alias = "ingest-generic-metrics")]

	#[serde(alias = "metrics_transactions")]
	#[serde(alias = "ingest-generic-metrics")]
	#[serde(alias = "metrics_transactions", alias = "ingest-generic-metrics")]

ref: Kafka topic configuration #3282

ref: Kafka topic configuration #3282

Conversation

lynnagara commented Mar 18, 2024

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