ref: Rationalize Kafka topic config #65793

lynnagara · 2024-02-26T17:46:04Z

This brings the concept of logical vs physical topic override to Sentry, which is fairly closely aligned with how topic management is done in Snuba.

Both the old way (KAFKA_TOPICS) and new way (KAFKA_TOPIC_OVERRIDES and KAFKA_TOPIC_TO_CLUSTER) will be supported until we have fully migrated over in production and all client code in this repository no longer reference the old variables.

fpacifici · 2024-02-27T02:20:54Z

src/sentry/conf/types/topic_definition.py

@@ -1,7 +1,39 @@
-from __future__ import annotations
-
+from enum import Enum


Shall we call this kafka_topic_definition ?

fpacifici · 2024-02-27T02:20:59Z

src/sentry/conf/types/topic_definition.py

 from typing import TypedDict


+class Topic(Enum):


Nice I like this.

fpacifici · 2024-02-27T02:24:28Z

src/sentry/conf/server.py

+# Mapping of default Kafka topic name to custom names
+KAFKA_TOPIC_OVERRIDES: Mapping[str, str] = {}
+
+# Mapping of default Kafka topic name to broker config
+KAFKA_TOPIC_TO_CLUSTER: Mapping[str, Mapping[str, TopicDefinition | None]] = {


In snuba these are called KAFKA_TOPIC_MAP and KAFKA_BROKER_CONFIG.
I think there would be value in consolidate the naming as well. It reduces the cognitive overhead when managing moving from one config (snuba) to the other (sentry).

Any chance we can name them in the same way? Even if this naming is more appropriate than the Snuba one.

I intentionally named it differently because it is not the same as Snuba at all. Here we map the topic to a cluster definition. Snuba does not go through any cluster definition and all the topic configs are mapped to the broker config. I think we might want to align them in the future but I'm not yet sure which way is better, and I want to avoid making this huge change in this PR right now.

src/sentry/conf/server.py

src/sentry/conf/types/consumer_definition.py

src/sentry/consumers/__init__.py

src/sentry/utils/batching_kafka_consumer.py

src/sentry/consumers/__init__.py

We should not special case the kafka config for this particular application feature.

lynnagara · 2024-02-28T20:53:52Z

src/sentry/conf/server.py

-    # When OUTCOMES_BILLING is None, it inherits from OUTCOMES and does not
-    # create a separate producer. Check ``track_outcome`` for details.
-    KAFKA_OUTCOMES_BILLING: None,


Moved this part into #65998 as it can be applied independently (and more safely) than the rest of this PR

fpacifici

Remembering how complicated this migration was in Snuba it will be critical to review the rollout plan. What is your strategy and which steps will you go through?
Also how are we going to deal with self hosted configuration ? As people may be using the old override system, we should make it clear it is deprecated and it is going to go away. In Snuba, if I remember correctly, we had loud warning log message claiming the config was deprecated.

fpacifici · 2024-03-01T03:12:39Z

src/sentry/conf/server.py

+
+# Mapping of default Kafka topic name to custom names
+KAFKA_TOPIC_OVERRIDES: Mapping[str, str] = {
+    "generic-metrics-subscription-results": KAFKA_GENERIC_METRICS_SUBSCRIPTIONS_RESULTS


I am not following why we need to set up the override this way for one topic only. Could you please provide some more context ?

src/sentry/conf/server.py

fpacifici · 2024-03-01T03:23:12Z

src/sentry/conf/types/kafka_definition.py

+import click
+
+
+class Topic(Enum):


It would be useful to add a docstring here explaining that these are the logical topics. This is because here is where people add topics, this is being used by many.

Added docstring. There is also a test now that asserts that these same values are present in sentry-kafka-schemas

src/sentry/conf/types/kafka_definition.py

fpacifici · 2024-03-01T03:26:34Z

src/sentry/conf/types/kafka_definition.py

+    # XXX: Eventually only Topic will be accepted here.
+    # For backward compatibility with getsentry, we must also
+    # support the physical override topic name (str, Callable[str], str)
+    # while the migration is taking place
+    topic: Required[Topic | str | Callable[[], str]]


So does this mean that this will be the logical name only ? If yes where does the override happens ?

override happens in settings and nowhere else, it will be fully removed from consumer definitions, and as a follow up all code will stop referencing those override values directly.

fpacifici · 2024-03-01T03:31:14Z

src/sentry/consumers/__init__.py

@@ -237,8 +239,8 @@ def ingest_events_options() -> list[click.Option]:
        },
    },
    "generic-metrics-subscription-results": {
-        "topic": settings.KAFKA_GENERIC_METRICS_SUBSCRIPTIONS_RESULTS,
-        "default_topic": "generic-metrics-subscription-results",
+        "topic": Topic.GENERIC_METRICS_SUBSCRIPTIONS_RESULTS,


Is there any specific reason this is the only topic being set via the Topic enum ?

I'll give the same answer as above - two reasons:

demonstration of how this works

it is the only topic that has schema validation turned on. In order to keep that happening, we need to know both the default and override topic there so it must use the new system.

fpacifici · 2024-03-01T03:34:00Z

src/sentry/consumers/__init__.py

+        real_topic = settings.KAFKA_TOPIC_OVERRIDES.get(default_topic, default_topic)
+    else:
+        # TODO: Deprecated, remove once this way is no longer used
+        if not isinstance(consumer_topic, str):


This condition seems counterintuitive. Was that any reason not to phrase it positively as:

if isinstance(consumer_topic, str): real_topic = consumer_topic else: ....

I did not write this code, I copied it from line 398 above, and don't want to refactor it unnecessarily given it will probably go away in a couple of days.

fpacifici · 2024-03-01T03:40:23Z

tests/sentry/conf/test_kafka_definitions.py

 from sentry.consumers import KAFKA_CONSUMERS
 from sentry.testutils.cases import TestCase


+def test_topic_definition() -> None:


I think we should also have a test to verify that all the topics in KAFKA_TOPIC_OVERRIDES are actually in the Topic enum.

This is always empty/default in dev/ci so it won't do much. But I added it anyway.

oh right, I guess this is more for getsentry

lynnagara · 2024-03-01T22:43:04Z

Remembering how complicated this migration was in Snuba it will be critical to review the rollout plan. What is your strategy and which steps will you go through? Also how are we going to deal with self hosted configuration ? As people may be using the old override system, we should make it clear it is deprecated and it is going to go away. In Snuba, if I remember correctly, we had loud warning log message claiming the config was deprecated.

Believe it or not I actually don't think this is going to be a particularly complicated or long migration. There is nothing to do in self-hosted since we don't really provide/document any way to to override topic configs. Though technically possible, I highly doubt anyone has figured out how to set up overrides and synchronize them across our codebases.

The next step will be to populate all the clusters and overrides in getsentry. I will do it one environment at a time to be safe, probably starting with the single tenants..

Once all the configuration is populated via both the old-way and new-way in all deployments, then we can switch all the code paths to stop referencing the old variables, and after that delete them.

fpacifici · 2024-03-02T01:16:30Z

There is nothing to do in self-hosted since we don't really provide/document any way to to override topic configs. Though technically possible, I highly doubt anyone has figured out how to set up overrides and synchronize them across our codebases.

We do provide a way to override sentry settings and it is in the public docs. That contains topics config. In Snuba we did take care of self hosted as we were used to receive github issues asking to allow the kafka settings customization (like authentication) which means people did customize the kafka config.

I'd really consider adding a simple log warning (after we migrated prod) to state the settings are deprecated. Then some time later remove them.

sentry-io · 2024-03-04T18:11:53Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ UnboundLocalError: cannot access local variable 'settings' where it is not associated with a value sentry.consumers in get_stream_processor View Issue

_{Did you find this useful? React with a 👍 or 👎}

getsentry-bot · 2024-03-04T19:49:31Z

PR reverted: 369b089

This reverts commit b273b38. Co-authored-by: wedamija <6288560+wedamija@users.noreply.github.com>

This reverts commit 369b089.

Redo of #65793 with fixed imports

ref: Rationalize Kafka topic config

40bf484

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 26, 2024

vercel bot deployed to Preview February 26, 2024 17:47 View deployment

lynnagara requested review from untitaker and fpacifici February 26, 2024 17:47

lynnagara and others added 2 commits February 26, 2024 10:01

add default

112473d

🛠️ apply pre-commit fixes

05cfb0a

vercel bot deployed to Preview February 26, 2024 18:06 View deployment

.

977df66

vercel bot deployed to Preview February 26, 2024 18:18 View deployment

cleanup topic autocreation

8025712

vercel bot deployed to Preview February 26, 2024 18:21 View deployment

add all the default topic names

3a510f2

vercel bot deployed to Preview February 27, 2024 01:10 View deployment

add topics

348c290

lynnagara requested a review from a team as a code owner February 27, 2024 01:24

comment no longer applies

9f7206e

vercel bot deployed to Preview February 27, 2024 01:29 View deployment

fpacifici reviewed Feb 27, 2024

View reviewed changes

untitaker approved these changes Feb 27, 2024

View reviewed changes

src/sentry/consumers/__init__.py Outdated Show resolved Hide resolved

fix comment

e7f1e9a

vercel bot deployed to Preview February 27, 2024 21:09 View deployment

lynnagara added 2 commits February 27, 2024 13:26

validate_schema is bool

7bb9d4f

Merge remote-tracking branch 'origin/master' into kafka-topics-cleanup

570c23d

vercel bot deployed to Preview February 27, 2024 21:31 View deployment

lynnagara added 4 commits February 28, 2024 12:34

ref: Fix billing kafka config

5635bee

We should not special case the kafka config for this particular application feature.

remove more special casing

4880de4

more cleanup of special cased things

334b83d

Merge branch 'fix-billing-kafka-config' into kafka-topics-cleanup

46d0259

lynnagara commented Feb 28, 2024

View reviewed changes

vercel bot deployed to Preview February 29, 2024 23:23 View deployment

lynnagara added 2 commits February 29, 2024 16:45

Merge remote-tracking branch 'origin/master' into kafka-topics-cleanup

95c3cce

add new ingest-events-dlq topic

b4e867a

vercel bot deployed to Preview March 1, 2024 00:49 View deployment

fpacifici reviewed Mar 1, 2024

View reviewed changes

fix typo, add test

b8e31eb

vercel bot deployed to Preview March 1, 2024 21:02 View deployment

add docstring

0daa510

vercel bot deployed to Preview March 1, 2024 21:18 View deployment

don't touch file unnecessarily

297db51

vercel bot deployed to Preview March 1, 2024 21:24 View deployment

add test

ffbb122

vercel bot deployed to Preview March 1, 2024 21:31 View deployment

yikes

59f93e9

vercel bot deployed to Preview March 1, 2024 22:14 View deployment

type checker

1a4f857

vercel bot deployed to Preview March 1, 2024 22:23 View deployment

fpacifici approved these changes Mar 2, 2024

View reviewed changes

lynnagara merged commit b273b38 into master Mar 4, 2024
51 checks passed

lynnagara deleted the kafka-topics-cleanup branch March 4, 2024 17:39

wedamija added the Trigger: Revert add to a merged PR to revert it (skips CI) label Mar 4, 2024

getsentry-bot added a commit that referenced this pull request Mar 4, 2024

Revert "ref: Rationalize Kafka topic config (#65793)"

369b089

This reverts commit b273b38. Co-authored-by: wedamija <6288560+wedamija@users.noreply.github.com>

lynnagara added a commit that referenced this pull request Mar 4, 2024

Revert "Revert "ref: Rationalize Kafka topic config (#65793)""

cf92413

This reverts commit 369b089.

lynnagara mentioned this pull request Mar 4, 2024

ref: Rationalize kafka config - take 2 #66242

Merged

lynnagara added a commit that referenced this pull request Mar 4, 2024

ref: Rationalize kafka config - take 2 (#66242)

2ad12d6

Redo of #65793 with fixed imports

github-actions bot locked and limited conversation to collaborators Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: Rationalize Kafka topic config #65793

ref: Rationalize Kafka topic config #65793

lynnagara commented Feb 26, 2024 •

edited

fpacifici Feb 27, 2024

fpacifici Feb 27, 2024

fpacifici Feb 27, 2024

lynnagara Feb 27, 2024

lynnagara Feb 28, 2024

fpacifici left a comment

fpacifici Mar 1, 2024

fpacifici Mar 1, 2024

lynnagara Mar 1, 2024

fpacifici Mar 1, 2024

lynnagara Mar 1, 2024 •

edited

fpacifici Mar 1, 2024

lynnagara Mar 1, 2024

fpacifici Mar 1, 2024

lynnagara Mar 1, 2024

fpacifici Mar 1, 2024

lynnagara Mar 1, 2024 •

edited

fpacifici Mar 2, 2024

lynnagara commented Mar 1, 2024 •

edited

fpacifici commented Mar 2, 2024

sentry-io bot commented Mar 4, 2024

getsentry-bot commented Mar 4, 2024

		@@ -1,7 +1,39 @@
		from __future__ import annotations

		from enum import Enum

ref: Rationalize Kafka topic config #65793

ref: Rationalize Kafka topic config #65793

Conversation

lynnagara commented Feb 26, 2024 • edited

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

fpacifici 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

Choose a reason for hiding this comment

lynnagara Mar 1, 2024 • edited

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

lynnagara Mar 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnagara commented Mar 1, 2024 • edited

fpacifici commented Mar 2, 2024

sentry-io bot commented Mar 4, 2024

Suspect Issues

getsentry-bot commented Mar 4, 2024

lynnagara commented Feb 26, 2024 •

edited

lynnagara Mar 1, 2024 •

edited

lynnagara Mar 1, 2024 •

edited

lynnagara commented Mar 1, 2024 •

edited