feat(profiles): Count processed profiles in billing metrics consumer #50047

jjbayer · 2023-05-31T10:03:33Z

Currently, accepted processed profiles are counted in Relay. To align processed transactions and processed profiles more consistently, we will change this by adding a tag to the transaction.duration metric (which we use for counting transactions), and defining num_processed_profiles := num_processed_transactions with profiles (see issue linked below for more information).

This PR needs to be deployed before getsentry/relay#2163 to prevent over-counting.

TODO:

Run end-to-end test

ref: getsentry/relay#2158

codecov · 2023-05-31T10:32:12Z

Codecov Report

Merging #50047 (06cec21) into master (f374b2f) will increase coverage by 0.17%.
The diff coverage is 94.44%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #50047      +/-   ##
==========================================
+ Coverage   80.93%   81.11%   +0.17%     
==========================================
  Files        4843     4843              
  Lines      203284   203296      +12     
  Branches    11253    11253              
==========================================
+ Hits       164537   164901     +364     
+ Misses      38498    38146     -352     
  Partials      249      249

Impacted Files	Coverage Δ
src/sentry/sentry_metrics/indexer/strings.py	`100.00% <ø> (ø)`
src/sentry/ingest/billing_metrics_consumer.py	`83.13% <94.44%> (+2.85%)`	⬆️

... and 47 files with indirect coverage changes

jjbayer · 2023-05-31T10:09:25Z

src/sentry/ingest/billing_metrics_consumer.py

+
+        items = {DataCategory.TRANSACTION: quantity}
+
+        if bucket_payload["tags"].get(self.profile_tag) == "true":


@getsentry/sns does production code still need to consider the case where we index tag values for performance metrics? In that case, we would have to resolve the string "true" to an integer ID here.

I noticed that for queries, we don't even consider that case anymore (see this PR).

On the other hand, indexing tag values is still on by default:

sentry/src/sentry/options/defaults.py

Lines 578 to 580 in 92cd949

# Flag to determine whether performance metrics indexer should index tag

# values or not

register("sentry-metrics.performance.index-tag-values", default=True)

~~I may be missing context here, but why do we compare with "true"?~~

Just reviewing https://github.com/getsentry/relay/pull/2165/files#diff-b2075df0875b0e516d6354525c4509faef97412b6d221bbe079fa52cb8d198f3R109.

for the sake of consistency i would use resolve_tag_value like everywhere else. it might be that we want to mass-refactor this sort of code in the future, and being able to find all code that deals with query results by searching for resolve calls definitely would help there

iker-barriocanal · 2023-06-01T08:56:15Z

src/sentry/sentry_metrics/indexer/strings.py

@@ -139,6 +139,8 @@
    "span.description": PREFIX + 249,
    "http.status_code": PREFIX + 250,
    "geo.country_code": PREFIX + 251,
+    # More Transactions
+    "has_profile": PREFIX + 300,


Why not +252?

I wanted to reserve some space for the span tags listed above, but it's probably a waste. Changed to 252 now.

iker-barriocanal · 2023-06-01T09:01:11Z

src/sentry/ingest/billing_metrics_consumer.py

+
+        items = {DataCategory.TRANSACTION: quantity}
+
+        if bucket_payload["tags"].get(self.profile_tag) == "true":


~~I may be missing context here, but why do we compare with "true"?~~

Just reviewing https://github.com/getsentry/relay/pull/2165/files#diff-b2075df0875b0e516d6354525c4509faef97412b6d221bbe079fa52cb8d198f3R109.

src/sentry/sentry_metrics/indexer/strings.py

tests/sentry/ingest/billing_metrics_consumer/test_billing_metrics_consumer_kafka.py

…ics_consumer_kafka.py

…g-consumer

Instead of counting processed profiles in two different places (see #2054), add a `has_profile` tag to the transaction counter metric, and define ``` processed_profiles := count(processed_transactions) WHERE has_profile = true ``` ## Changes This PR contains the following changes: * Revert PRs around counting profiles. * Add `has_profile(s)` tag on `transaction.duration` metric. #2165 * Update metrics billing consumer to emit `accepted` outcome for profiles. getsentry/sentry#50047 ## Caveats With this PR, rate limits will be applied consistently if they are issued for `DataCategory.Transaction`. However, if a rate limit (e.g. spike protection) is issued _only_ for `DataCategory.Profile`, it will not be enforced. ## Rollout Order 1. Merge and deploy getsentry/sentry#50047. The billing metrics consumer will now listen for profiles, but not receive any. 3. Deploy processing Relays. Profile counts will go down, because PoP-Relays are not sending the `has_profile` tag yet. 4. Deploy PoP Relays. Profile counts should go back to normal. ## In case of rollback 1. First revert the sentry change (getsentry/sentry#50047) and deploy it to stop counting profiles. 2. Then revert the Relay change (this PR) and deploy to processing Relays and PoPs. ref: #2158 --------- Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>

* master: (52 commits) feat(replays): Add dead click issue detector (#50149) fix(sourcemaps): Improve metrics collection (#50293) ref(dynamic-sampling): Implement new abstraction for models (#50227) Revert "feat(backpressure): Proof of concept for queue monitoring service (#49872)" fix(dynamic-sampling): Remove duplicated queue declaration (#50283) feat(backpressure): Proof of concept for queue monitoring service (#49872) feat: Add pages for org auth tokens (#50225) feat(strings): Add span.category tag (#50286) feat(profiles): Count processed profiles in billing metrics consumer (#50047) feat(strings): Add `transaction.method` tag (#50237) ref(txnames): Increase sample size (#50278) ref(framework-suggestion-modal): Sort platforms by popularity (#50235) ref(hc): Only set the organizationmembermapping.member_id for monolith (#49789) ref(hc): Region name is set for organization mappings (#50257) fix(alert-details): Prevent environments call from being cancelled (#50270) fix(post-process-group): retry retrieving events from eventstore up to 3 times (#50210) fix(functions): Ensure selected functions projects are returned (#50194) styles(functions): Handle function name overflow in suspect functions (#50251) feat(functions): Add tooltip to slowest functions score (#50167) ref(routes): Avoid named exports for routes for now (#50255) ...

jjbayer added 5 commits May 30, 2023 13:44

draft

412e7aa

ref

7085a3d

fix: Use integer tag key

e5c532c

test

7ed7eb2

fix

92cd949

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 31, 2023

vercel bot deployed to Preview May 31, 2023 10:04 View deployment

jjbayer mentioned this pull request May 31, 2023

ref(profiles): Counting profiles, V2 getsentry/relay#2163

Merged

jjbayer commented May 31, 2023

View reviewed changes

jjbayer marked this pull request as ready for review May 31, 2023 14:30

jjbayer requested a review from a team May 31, 2023 14:30

jjbayer requested a review from a team as a code owner May 31, 2023 14:30

jjbayer mentioned this pull request May 31, 2023

[EPIC] Ensure processed transactions always exist for processed profiles getsentry/relay#2158

Closed

jjbayer requested review from a team May 31, 2023 14:35

iker-barriocanal approved these changes Jun 1, 2023

View reviewed changes

jjbayer commented Jun 1, 2023

View reviewed changes

src/sentry/sentry_metrics/indexer/strings.py Outdated Show resolved Hide resolved

Update src/sentry/sentry_metrics/indexer/strings.py

b5086ce

vercel bot deployed to Preview June 1, 2023 11:38 View deployment

jjbayer commented Jun 1, 2023

View reviewed changes

tests/sentry/ingest/billing_metrics_consumer/test_billing_metrics_consumer_kafka.py Outdated Show resolved Hide resolved

Update tests/sentry/ingest/billing_metrics_consumer/test_billing_metr…

c6b94c6

…ics_consumer_kafka.py

vercel bot deployed to Preview June 1, 2023 12:11 View deployment

jjbayer added 2 commits June 4, 2023 18:04

Merge branch 'master' into ref/profiles-billing-consumer

5f9f7b5

ref: Use resolve_tag_value

940ed1d

vercel bot deployed to Preview June 4, 2023 16:24 View deployment

ref: Don't resolve when no profiles

881e5bc

vercel bot deployed to Preview June 5, 2023 06:50 View deployment

mypy

e18df9f

vercel bot deployed to Preview June 5, 2023 07:02 View deployment

jjbayer added 3 commits June 5, 2023 09:25

test

20a21c1

Merge remote-tracking branch 'origin/master' into ref/profiles-billin…

e54184d

…g-consumer

test

06cec21

vercel bot deployed to Preview June 5, 2023 07:31 View deployment

jjbayer merged commit 22a4550 into master Jun 5, 2023
54 checks passed

jjbayer deleted the ref/profiles-billing-consumer branch June 5, 2023 09:08

github-actions bot locked and limited conversation to collaborators Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(profiles): Count processed profiles in billing metrics consumer #50047

feat(profiles): Count processed profiles in billing metrics consumer #50047

jjbayer commented May 31, 2023 •

edited

Loading

codecov bot commented May 31, 2023 •

edited

Loading

jjbayer May 31, 2023

iker-barriocanal Jun 1, 2023 •

edited

Loading

untitaker Jun 4, 2023

iker-barriocanal Jun 1, 2023

jjbayer Jun 1, 2023

iker-barriocanal Jun 1, 2023 •

edited

Loading


		items = {DataCategory.TRANSACTION: quantity}

		if bucket_payload["tags"].get(self.profile_tag) == "true":

	# Flag to determine whether performance metrics indexer should index tag
	# values or not
	register("sentry-metrics.performance.index-tag-values", default=True)

feat(profiles): Count processed profiles in billing metrics consumer #50047

feat(profiles): Count processed profiles in billing metrics consumer #50047

Conversation

jjbayer commented May 31, 2023 • edited Loading

codecov bot commented May 31, 2023 • edited Loading

Codecov Report

jjbayer May 31, 2023

Choose a reason for hiding this comment

iker-barriocanal Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

untitaker Jun 4, 2023

Choose a reason for hiding this comment

iker-barriocanal Jun 1, 2023

Choose a reason for hiding this comment

jjbayer Jun 1, 2023

Choose a reason for hiding this comment

iker-barriocanal Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

jjbayer commented May 31, 2023 •

edited

Loading

codecov bot commented May 31, 2023 •

edited

Loading

iker-barriocanal Jun 1, 2023 •

edited

Loading

iker-barriocanal Jun 1, 2023 •

edited

Loading