Allow users to specify Kafka topic name prefix in values.yaml #1544
Conversation
fix: user-create-job hook does not create user
…into develop # Conflicts: # charts/sentry/values.yaml
|
Hi! How to check? Could you write example values.yaml. |
Hello, an example would just be the prefix you want. i.e. "sentry.dev." |
|
I test with: and get error pod logs: and and other pod get same error |
I think I commented above the section but you need to adjust the Kafka subchart’s provision topic names with prefixed ones as well. |
|
Could you write example values.yaml? |
Hello, I updated the example yaml and ran the E2E. I'll revert the commit now |
This reverts commit 6db7087.
|
git clone you repo install get error |
What's the error message? They are working in mine.. |
|
@patsevanton I did a dry-run and inspected the deployment manifest, seemed to be an issue with template and I corrected it. |
|
works, but may be comments code? |
Done! |
and my config: |
Updated again.. https://paste.gg/p/anonymous/4e7302ba7f2946cd8fdf6969f77fde0f is generated dry-run manifest I observed. It looks like prefixes are getting updated properly now. |
|
Do you test the code before you submit it? |
Fixed one more issue related to argument name then it should be good to go |
|
I install sentry by command All pods are in the Running state @Mokto @adonskoy @MemberIT @2005wind @iamyogeshg @paulDashkevich could you help to review? |
| spans: "{{ default "" .Values.kafkaTopicOverrides.prefix }}snuba-spans" | ||
| metrics_summaries: "{{ default "" .Values.kafkaTopicOverrides.prefix }}snuba-metrics-summaries" | ||
| cogs: "{{ default "" .Values.kafkaTopicOverrides.prefix }}shared-resources-usage" | ||
| feedback: "{{ default "" .Values.kafkaTopicOverrides.prefix }}ingest-feedback-events" |
There was a problem hiding this comment.
Could you tell me, what is this topic ingest-feedback-events? I don't see the provisioning of this topic here
There was a problem hiding this comment.
#1359 (comment) has ingest-feedback-events, if it's not needed then it could be removed.
There was a problem hiding this comment.
Seems like it is in the kafka definition, we may need to provision it?
|
@nadecancode, am I correct in understanding that prefixes are added only to the following topics: topics:
metrics_sessions: "test-for-commit-ingest-metrics"
events: "test-for-commit-ingest-attachments"
transactions: "test-for-commit-ingest-transactions"
outcomes: "test-for-commit-ingest-outcomes"
outcomes_billing: "test-for-commit-ingest-outcomes"
metrics_generic: "test-for-commit-ingest-performance-metrics"
profiles: "test-for-commit-profiles"
replay_events: "test-for-commit-ingest-replay-events"
replay_recordings: "test-for-commit-ingest-replay-recordings"
monitors: "test-for-commit-ingest-monitors"
spans: "test-for-commit-snuba-spans"
metrics_summaries: "test-for-commit-snuba-metrics-summaries"
cogs: "test-for-commit-shared-resources-usage"
feedback: "test-for-commit-ingest-feedback-events"and not to all the ones that are created in Kafka here? If that's the case, then we should add a comment for the corresponding topics, something like this: @@ -2058,7 +2058,7 @@ kafka:
config:
"message.timestamp.type": LogAppendTime
- name: profiles-call-tree
- - name: ingest-replay-events
+ - name: ingest-replay-events # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
config:
"message.timestamp.type": LogAppendTime
"max.message.bytes": "15000000"
@@ -2095,20 +2095,20 @@ kafka:
- name: snuba-dead-letter-generic-events
- name: snuba-dead-letter-querylog
- name: snuba-dead-letter-group-attributes
- - name: ingest-attachments
- - name: ingest-transactions
+ - name: ingest-attachments # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
+ - name: ingest-transactions # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
- name: ingest-events
## If the number of exceptions increases, it is recommended to increase the number of partitions for ingest-events
# partitions: 1
- - name: ingest-replay-recordings
- - name: ingest-metrics
- - name: ingest-performance-metrics
- - name: ingest-monitors
- - name: profiles
+ - name: ingest-replay-recordings # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
+ - name: ingest-metrics # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
+ - name: ingest-performance-metrics # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
+ - name: ingest-monitors # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
+ - name: profiles # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
- name: ingest-occurrences
- - name: snuba-spans
- - name: shared-resources-usage
- - name: snuba-metrics-summaries
+ - name: snuba-spans # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
+ - name: shared-resources-usage # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefix
+ - name: snuba-metrics-summaries # Set here same topic prefix if set .Values.kafkaTopicOverrides.prefixOr group them under one comment by highlighting them in a separate section. |
No, all topics provisioned need to be prefixed. Relay only uses some of the topics so we specify manually in yaml. Snubs and sentry use python so I iterated all topics in enum and overlaid an override. |
|
why are we not using all these kafka topics? what is needed to use all these kafka topics? maybe we need to update sentry? |
I believe it’s more like relay only needs these topics, since its role is closer to a proxy (hence “relay”) |
There was a problem hiding this comment.
There are dozens of topics and consumers in Sentry, I don’t see a universal solution so that a prefix can be assigned to all topic names, with this PR this can only be done for some topics
I believe that this PR is blocked by another problem - we first need our own job to create topics in Kafka, because the current implementation has many shortcomings and then adding features like this
I am not sure about this, that “dozen of topics” are basically all in the Sentry Topic enum so I don’t see a problem with assigning prefix through iteration. The topics that we provision via values.yaml should cover all topics that we need to get Sentry up and running. The main difference that people need to keep in mind is some of Sentry’s components do not use all topics provisioned. |
|
I also tested this PR in my company’s deployment, there is no problem with getting that instance running and ingesting transactions, errors, etc. |
Sorry I just reread the change request - I might have been less clear than I intended to. So basically, all topics in the provisioning section need to be prefixed, they are ALL being used. But it does not make sense to comment them individually when all of them need to be treated the same way. Would it be better if we just add a comment at the top of |
|
@nadecancode As for the whole PR, unfortunately, I can't recommend approving or not approving it as a whole, because I lack the knowledge of how Sentry and its components interact with Kafka. I just tried to point out the parts of the code that, in my opinion, could be improved. |
Thank you, @Mokto may I ask if it’s possible to get it merged? I already tested the change in my company’s sentry instance and it’s working as intended - but we would like this to go to the upstream so we don’t have to maintain a separate helm chart. |
|
There are conflicts right now. Thanks |
|
👋 Hi, @nadecancode, |
Resolved |
Instructed per #1359 (comment), this PR adds a new section called
kafkaTopicOverrides.prefixfor users to specify their own kafka topic name prefix. This can avoid Sentry using an overly generic kafka topic name and allows for more rooms of customizations.