feat: Remove default topic duplication and add kafka topic test #3361

lynnagara · 2024-03-29T17:06:49Z

Fixes related to INC-696, which accidentally had ingest-generic-metrics as the default topic name alias instead of ingest-performance-metrics.

This PR attempts to do 2 things

Move defining the default topic name to a single place rather than 2
Check the default topic names against those registered against sentry-kafka-schemas to ensure they are aligned with what the other codebases have registered as defaults

Needs getsentry/sentry-kafka-schemas#242

Note that outcomes-billing no longer defaults to outcomes and ingest-performance-metrics no longer defaults to ingest-metrics, as there is no scenario where they should share topics anymore.

Fixes related to INC-696, which accidentally had `ingest-generic-metrics` as the default topic name alias instead of `ingest-performance-metrics`. This PR attempts to do 2 things - Move defining the default topic name to a single place rather than 2 - Check the default topic names against those registered against sentry-kafka-schemas to ensure they are aligned with what the other codebases have registered as defaults

lynnagara · 2024-04-01T17:57:31Z

a bit stumped by the failing integration test, any ideas how to fix it?

jjbayer · 2024-04-02T11:03:47Z

relay-kafka/src/config.rs

+define_topic_assignments! {
+    TopicAssignments {
+        events: (KafkaTopic::Events, "ingest-events"),
+        attachments: (KafkaTopic::Attachments, "ingest-attachments"),
+        transactions: (KafkaTopic::Transactions, "ingest-transactions"),
+        outcomes: (KafkaTopic::Outcomes, "outcomes"),
+        outcomes_billing: (KafkaTopic::OutcomesBilling, "outcomes-billing"),
+        metrics_sessions: (KafkaTopic::MetricsSessions, "ingest-metrics"),
+        metrics_generic: (KafkaTopic::MetricsGeneric, "ingest-performance-metrics"),
+        profiles: (KafkaTopic::Profiles, "profiles"),
+        replay_events: (KafkaTopic::ReplayEvents, "ingest-replay-events"),
+        replay_recordings: (KafkaTopic::ReplayRecordings, "ingest-replay-recordings"),
+        monitors: (KafkaTopic::Monitors, "ingest-monitors"),
+        spans: (KafkaTopic::Spans, "snuba-spans"),
+        metrics_summaries: (KafkaTopic::MetricsSummaries, "snuba-metrics-summaries"),
+        cogs: (KafkaTopic::Cogs, "shared-resources-usage"),
    }
 }


jjbayer · 2024-04-02T11:05:25Z

relay-kafka/src/config.rs

-            attachments: "ingest-attachments".to_owned().into(),
-            transactions: "ingest-transactions".to_owned().into(),
-            outcomes: "outcomes".to_owned().into(),
-            outcomes_billing: None,


How are we handling this special case? I assume this is the reason why integration tests are failing, see

relay/relay-kafka/src/config.rs

Line 138 in 94cb3df

KafkaTopic::OutcomesBilling => self.outcomes_billing.as_ref().unwrap_or(&self.outcomes),

I'm not handling it at all. This is no longer treated as a special case in any of our other services (see https://github.com/getsentry/sentry/blob/d414afaab6fcd22e8609d0a885e25cad0752b83b/src/sentry/conf/server.py#L3477 and https://github.com/getsentry/sentry-kafka-schemas/blob/main/topics/outcomes-billing.yaml for example). If someone reaalllly wanted to share the topic technically still they could accomplish it via overriding outcomes-billing back to outcomes in relay config (but that's not really something we'd do anymore). It only ever existed for dev and dev was updated to just match prod on this recently.

Does it break self hosted?

It doesn't. Self hosted runs a billing consumer too. https://github.com/getsentry/self-hosted/blob/c4ae491929170c075bea608941b227a8db81eec2/docker-compose.yml#L268-L270.

Getsentry already produces to that topic and it gets written to clickhouse similarly to before.

jjbayer · 2024-04-03T10:41:22Z

a bit stumped by the failing integration test, any ideas how to fix it?

I picked a few of the failing tests randomly and they all passed on my machine, must have something to with the topic configuration. I'll take another look.

Dav1dde

Love the changes!

Dav1dde · 2024-04-03T10:47:33Z

relay-kafka/src/config.rs

+}
+
+define_topic_assignments! {
+    TopicAssignments {


Can we get the documentation back? It should be possible to include it in the macro and just generate it on the fields using #[doc = $foo].

Sounds good, added back the original descriptions

Dav1dde · 2024-04-03T10:48:13Z

relay-kafka/src/config.rs

-            attachments: "ingest-attachments".to_owned().into(),
-            transactions: "ingest-transactions".to_owned().into(),
-            outcomes: "outcomes".to_owned().into(),
-            outcomes_billing: None,


Does it break self hosted?

relay-kafka/src/config.rs

Dav1dde · 2024-04-03T10:55:01Z

a bit stumped by the failing integration test, any ideas how to fix it?

Possibly to do with how it auto generates topic names, maybe the mapping is broken?

jjbayer · 2024-04-03T15:45:03Z

a bit stumped by the failing integration test, any ideas how to fix it?

I pushed an update to the test topics config, let's see if that makes tests pass 🤞

Dav1dde · 2024-04-03T16:34:16Z

I pushed an update to the test topics config, let's see if that makes tests pass 🤞

I am a bit worried, this is now officially a breaking change. If we have it configured that way, somebody else will probably too (self hosted?).

Co-authored-by: David Herberth <[email protected]>

jjbayer

I am a bit worried, this is now officially a breaking change. If we have it configured that way, somebody else will probably too (self hosted?).

Agreed, we should restore the existing topic aliases (see comment).

jjbayer · 2024-04-04T07:17:58Z

relay-kafka/src/config.rs

+        metrics_sessions: (KafkaTopic::MetricsSessions, "ingest-metrics"),
+        metrics_generic: (KafkaTopic::MetricsGeneric, "ingest-performance-metrics"),


I think we need to bring back the metrics and metrics_transactions aliases to ensure backward compatibility. Maybe we can tweak the macro to allow multiple aliases.

What do we need the backward compatibility for? Do we think that self-hosted users would have used these aliases? And is it the intention to support that forever? (it is not part of the default self-hosted configuration)

I guess to not cause a breaking change if it is unnecessary? We can also call it out as a breaking change in the changelog, the number of users who configure topics manually is probably close to zero.

I updated the changelog in this PR. Did you want one calling it out in self-hosted too?

My personal feelings on this are that making this more complicated in order to maintain compatibility with something that probably no one has used is not really worth it. I'm not sure how this is generally handled in Relay, but in sentry and snuba codebases at least, breaking compatibility is generally fine as long as we call it out (and we did already break compatibility when this same change was shipped there)

jjbayer

I like the changelog entry, it's very clear. If we are sure that this does not break default self-hosted or any other deployments maintained by us, ship it!

lynnagara requested a review from a team as a code owner March 29, 2024 17:06

lynnagara added 2 commits March 29, 2024 15:58

bump sentry-kafka-schemas

8af08ce

attachment also not defined

27f3186

jjbayer reviewed Apr 2, 2024

View reviewed changes

iker-barriocanal assigned lynnagara Apr 2, 2024

Dav1dde approved these changes Apr 3, 2024

View reviewed changes

test: update topic config

ce978ba

Update relay-kafka/src/config.rs

0c80e8f

Co-authored-by: David Herberth <[email protected]>

jjbayer requested changes Apr 4, 2024

View reviewed changes

lynnagara added 7 commits April 5, 2024 12:29

add breaking change to changelog

54bc4a5

preserve original docs

5fc654e

changelog

5999240

is this what is needed?

7b9e6e0

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

793e296

i have no idea what i'm doing

6e90c64

maybe this?

3762fef

lynnagara changed the title ~~ref: Remove default topic duplication and add kafka topic test~~ feat: Remove default topic duplication and add kafka topic test Apr 5, 2024

maybe it doesn't like the trailing slash?

1c0a0e5

jjbayer approved these changes Apr 8, 2024

View reviewed changes

lynnagara added 3 commits April 9, 2024 12:28

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

b03a997

bad merge

b5d676c

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

6e554c4

lynnagara enabled auto-merge (squash) April 9, 2024 23:29

lynnagara disabled auto-merge April 9, 2024 23:29

lynnagara merged commit c7d27ba into master Apr 10, 2024
20 checks passed

lynnagara deleted the define-topic-config branch April 10, 2024 18:21

jjbayer mentioned this pull request May 7, 2024

ERROR relay_server::services::project #3553

Closed

This was referenced Jun 7, 2024

Sentry stops logging issues for projects after some time getsentry/self-hosted#3016

Closed

Sentry Relay - Breaking Change in 24.4.0 removal of metrics_transactions alias from relay - how does this affect the Sentry chart? sentry-kubernetes/charts#1306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Remove default topic duplication and add kafka topic test #3361

feat: Remove default topic duplication and add kafka topic test #3361

lynnagara commented Mar 29, 2024 •

edited

Loading

lynnagara commented Apr 1, 2024

jjbayer Apr 2, 2024

jjbayer Apr 2, 2024

lynnagara Apr 2, 2024 •

edited

Loading

Dav1dde Apr 3, 2024

lynnagara Apr 3, 2024 •

edited

Loading

jjbayer commented Apr 3, 2024

Dav1dde left a comment

Dav1dde Apr 3, 2024

lynnagara Apr 5, 2024

Dav1dde Apr 3, 2024

Dav1dde commented Apr 3, 2024

jjbayer commented Apr 3, 2024

Dav1dde commented Apr 3, 2024

jjbayer left a comment

jjbayer Apr 4, 2024

lynnagara Apr 4, 2024 •

edited

Loading

jjbayer Apr 5, 2024

lynnagara Apr 5, 2024

jjbayer left a comment

		metrics_sessions: (KafkaTopic::MetricsSessions, "ingest-metrics"),
		metrics_generic: (KafkaTopic::MetricsGeneric, "ingest-performance-metrics"),

feat: Remove default topic duplication and add kafka topic test #3361

feat: Remove default topic duplication and add kafka topic test #3361

Conversation

lynnagara commented Mar 29, 2024 • edited Loading

lynnagara commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnagara Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnagara Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

jjbayer commented Apr 3, 2024

Dav1dde 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

Dav1dde commented Apr 3, 2024

jjbayer commented Apr 3, 2024

Dav1dde commented Apr 3, 2024

jjbayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnagara Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer left a comment

Choose a reason for hiding this comment

lynnagara commented Mar 29, 2024 •

edited

Loading

lynnagara Apr 2, 2024 •

edited

Loading

lynnagara Apr 3, 2024 •

edited

Loading

lynnagara Apr 4, 2024 •

edited

Loading