feat(subscriptions): Adds metrics topics to topics validation #2326
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2326 +/- ##
==========================================
- Coverage 92.75% 92.68% -0.07%
==========================================
Files 559 559
Lines 25736 25736
==========================================
- Hits 23871 23854 -17
- Misses 1865 1882 +17
Continue to review full report at Codecov.
|
ahmedetefy
force-pushed
the
revert-2313-revert-2303-cra-metrics/validate_metrics_topics
branch
from
ec5c538
to
c3b5db7
Compare
ahmedetefy
changed the base branch from
master
to
revert-2314-revert-2222-cra-metrics/add-subscription-consumer
ahmedetefy
force-pushed
the
revert-2314-revert-2222-cra-metrics/add-subscription-consumer
branch
from
7c3150b
to
d6cb024
Compare
ahmedetefy
force-pushed
the
revert-2313-revert-2303-cra-metrics/validate_metrics_topics
branch
from
c3b5db7
to
1e1b276
Compare
snuba/settings/validation.py
Outdated
| @@ -36,6 +38,14 @@ def _validate_settings(locals: Mapping[str, Any]) -> None: | |||
| "transactions-subscription-results", | |||
| "sessions-subscription-results", | |||
| } | |||
| if settings.ENABLE_METRICS_SUBSCRIPTIONS: | |||
There was a problem hiding this comment.
You don't need to check this setting here, you can just add it to the main topic_names dictionary. Putting it on this list doesn't really have any effect, just allows those topics to be configured via KAFKA_TOPIC_MAP and KAFKA_BROKER_CONFIG without erroring.
There was a problem hiding this comment.
Fwiw I'm not sure why the original PR was reverted earlier - it should be harmless
There was a problem hiding this comment.
I am not entirely sure as well, but it seemed that the consumers kept restarting according to what @fpacifici told me until I reverted that PR so I assumed it must be that its checking for topic names that don't exist in prod somehow?
There was a problem hiding this comment.
I don't think so. Can you link/paste the actual error message you were seeing to confirm?
AFAIK the issue the other day was actually to do with this PR https://github.com/getsentry/snuba/pull/2222/files
If you pass a commit log topic to the consumer like that does it will automatically start publishing offsets to that topic. Is it possible the metrics commit log didn't exist in prod yet?
There was a problem hiding this comment.
@lynnagara Do you reckon I should try to deploy this without the feature flag check since these are not production consumers?
There was a problem hiding this comment.
https://github.com/getsentry/snuba/blob/master/snuba/settings/validation.py#L40-L46
These are the only lines where topic_names is used.. you can see there is no need for the actual topics to exist.
There was a problem hiding this comment.
Let's confirm what happened last week. I don't think we should be putting feature flags around everything "just in case"
|
Nit: This PR is not actually a revert of 2313 right now since you have changed stuff so I would not describe it as such. |
ahmedetefy
force-pushed
the
revert-2314-revert-2222-cra-metrics/add-subscription-consumer
branch
from
d6cb024
to
6fec959
Compare
|
@ahmedetefy This PR has changed quite a lot since earlier reviewed. Is there a reason it now does a lot more than changing the validation.py file? |
|
Also how come it's not based on master? |
|
@lynnagara Yeah it just needed to be rebased |
ahmedetefy
changed the base branch from
revert-2314-revert-2222-cra-metrics/add-subscription-consumer
to
master
ahmedetefy
force-pushed
the
revert-2313-revert-2303-cra-metrics/validate_metrics_topics
branch
from
a4b4dd8
to
b82e523
Compare
They have been created: |
…te_metrics_topics
ahmedetefy
deleted the
revert-2313-revert-2303-cra-metrics/validate_metrics_topics
branch
Reverts #2313