Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(metrics): Change metrics topic name #2089

Merged
merged 2 commits into from
Sep 28, 2021
Merged

Conversation

MeredithAnya
Copy link
Member

getsentry/sentry#28431 Adds a consumer for the ingest-metrics topic that then translates the raw metrics data of strings for all the metric names, tag keys and tag values to integers. The translated messages will go in the snuba-metrics topic so that ClickHouse can store the metrics data.

@MeredithAnya MeredithAnya requested a review from a team as a code owner September 9, 2021 18:57
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2021

Codecov Report

Merging #2089 (c81c3ca) into master (92e15e4) will decrease coverage by 0.06%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2089      +/-   ##
==========================================
- Coverage   91.11%   91.04%   -0.07%     
==========================================
  Files         510      510              
  Lines       22346    22346              
==========================================
- Hits        20361    20346      -15     
- Misses       1985     2000      +15     
Impacted Files Coverage Δ
snuba/settings/validation.py 65.51% <ø> (ø)
snuba/utils/streams/topics.py 81.81% <50.00%> (ø)
snuba/cli/__init__.py 70.00% <0.00%> (-30.00%) ⬇️
snuba/cli/api.py 17.39% <0.00%> (-26.09%) ⬇️
snuba/cli/bootstrap.py 0.00% <0.00%> (-10.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92e15e4...c81c3ca. Read the comment docs.

@lynnagara
Copy link
Member

Does this PR depend on the Sentry one first or are they independent?

snuba-metrics might need to be created with LogAppendTime if we want subscriptions on it later

nikhars
nikhars previously approved these changes Sep 14, 2021
@nikhars nikhars dismissed their stale review September 14, 2021 18:04

We should add LogAppendTime as suggested by Lyn

@MeredithAnya
Copy link
Member Author

Does this PR depend on the Sentry one first or are they independent?

snuba-metrics might need to be created with LogAppendTime if we want subscriptions on it later

@lynnagara They can be independent since using this topic requires manually running the consumer in sentry (that ultimately produces to the snuba-metrics topic) and is still in development right now

I believe we will want subscriptions on it, how do I create it with LogAppendTime?

config = {Topic.EVENTS: {"message.timestamp.type": "LogAppendTime"}}
config = {
Topic.EVENTS: {"message.timestamp.type": "LogAppendTime"},
Topic.METRICS: {"message.timestamp.type": "LogAppendTime"},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lynnagara was this all I needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks. Has this topic already being created in production? If yes we should make sure it has this configuration, if no we need to create it with this configuration.

Copy link
Member Author

@MeredithAnya MeredithAnya Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lynnagara

Has this topic already being created in production?

not to my knowledge, is this something we can open a PR for? or is it a request to ops?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can wait till when we will run the consumer in prod. Anyway there is no Clickhouse storage. Let's get everything working properly in dev first

@MeredithAnya MeredithAnya force-pushed the metrics/SNS-397 branch 2 times, most recently from afcc06a to 64fe3d4 Compare September 24, 2021 18:53
@MeredithAnya MeredithAnya force-pushed the metrics/SNS-397 branch 2 times, most recently from 4caae1d to c81c3ca Compare September 27, 2021 23:26
@MeredithAnya MeredithAnya merged commit 63db117 into master Sep 28, 2021
@MeredithAnya MeredithAnya deleted the metrics/SNS-397 branch September 28, 2021 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants