ref(config): Introduce multi-topic config, deprecate old fields#663
Conversation
ref STREAM-1042 We want to eventually support multiple topics being consumed. This PR adds the new structure.
…ic again, deprecate old fields
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The upkeep tests built Config literals (overriding kafka_deadletter_topic after normalization, or skipping normalization entirely), so kafka_producer_config() panicked because the deadletter topic was never registered in kafka_topics. Introduce create_integration_config_from_base(base) as the single normalized-config builder and rebuild the other integration helpers on top of it. Tests now pass their overrides via the base config so normalization runs after all fields are set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The legacy normalization path keys topics and clusters by name. A deadletter topic configured with the same name as the consumed topic previously collapsed silently into the main topic's entry (via entry().or_insert_with()), so the deadletter producer would resolve to the main cluster instead of the deadletter cluster — misrouting deadletter messages. Use the insert return value to reject that collision with a clear error, and assert the remaining internal inserts (clusters, main topic) never overwrite an existing entry. The intentional retry-topic alias of the main topic is left as-is. ref STREAM-1042 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The retry topic may alias the main topic (retries get re-enqueued there), but aliasing the deadletter topic silently gave the retry topic the deadletter cluster/role via entry().or_insert_with(). Reject that collision during normalization. ref STREAM-1042 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The forwarding producer reuses the deadletter cluster's credentials and only overrides bootstrap.servers, so forwarding to a different cluster with its own auth can fail to publish. Behavior is unchanged for compatibility, but we now warn when the forward cluster differs from the deadletter cluster and the latter has auth configured. ref STREAM-1042 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fb61fe7. Configure here.
The upkeep producer connects to the deadletter topic's cluster but is also reused to publish retries to the retry topic (or the consumed topic when none is configured). A single producer reaches only one cluster, so a deadletter topic on a different cluster than the retry target silently misroutes retries to the wrong brokers. Reject that during normalization. Replaces a bogus test that asserted the deadletter topic could live on its own cluster, which the shared-producer implementation cannot actually support. ref STREAM-1042 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
In the new format kafka_retry_topic was never registered as a topic, so a retry topic on a different cluster would silently fall through to the deadletter producer's cluster with no validation. Require the retry target to be a declared topic so its cluster is known and checked against the deadletter cluster. ref STREAM-1042 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kafka_consumer_group is documented as deprecated and participates in the legacy/new-format mutual-exclusivity check, but unlike the other deprecated fields it emitted no warning when set on its own. Add the matching deprecation warning. ref STREAM-1042 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
evanh
left a comment
There was a problem hiding this comment.
Why not have normalize_and_validate be called in the default() function directly? Why make it separate?
because the flow is like this:
that's how it works in
if we normalize before figment, or validate before it, we'll just not validate or normalize any user input. same in tests: we get a default config, overwrite whatever is necessary, then validate/normalize. |
…116758) See getsentry/taskbroker#667 for details. Same kafka config migration as the self-hosted change, but for the `ingest-profiles` taskbroker in devservices. Deprecation warnings land as per getsentry/taskbroker#663 `ingest-profiles` runs in raw mode, and raw mode now requires an explicit retry topic: raw payloads aren't activations, so retries can't loop back into the `profiles` topic. Retries now go to the main `taskworker` topic so the existing taskbroker picks them up instead of running another broker. Depends on getsentry/taskbroker#668, which wires up per-topic raw mode and enforces the retry-topic requirement. Once that ships, the current devenv config breaks without this change. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

ref STREAM-1042
We want to eventually support consuming multiple topics from a single taskbroker replica; https://app.notion.com/p/sentry/Multi-topic-management-for-taskbroker-3728b10e4b5d80adba06e127c5fac612 -- this is mainly for raw mode, so that we can consume multiple raw topics.
This is the first part that migrates the config to the new format. It is fully backwards compatible and old fields will print deprecation warnings. Multi-topic support is not actually implemented and defining more than one topic will result in a validation error.
Fields are migrated in
Config::validate(), and it's renamed tonormalize_and_validate.consumable_topic()has been introduced to make migration of the application less miserable, but when the actual support for multi-topic is implemented, that method has to go away.Config::default()no longer produces a valid config. Instead, one has to runConfig::default()(what figment uses), then always callnormalize_and_validateafterwards to get a config that the rest of the app code can actually deal with.What has been migrated is:
kafka_deadletter_clusteris now just... a cluster inkafka_clusters)What has not been migrated:
kafka_consume_retry_topichas been removed entirely because the migration is too complicated. It is not currently used and I think until we have proper multi-topic support, we can work without it.Other changes:
While grinding through bugbot comments, I had to add a ton of extra validation just to deal with the fact that we're sharing a producer across many topics. The old config was more restrictive in what kind of values it allowed to set (for example, retry topic didn't have a dedicated retry cluster setting), and the new format structurally allows for too many possibilities that are not actually handled in code.
I think we should follow up and actually create separate producers for DLQ vs retry topic vs demoted namespaces, then this unnecessary restriction also goes away. It's also a footgun anyway since the k8s deployment templates don't validate against this either.