-
Notifications
You must be signed in to change notification settings - Fork 119
feat(outcomes): Have relay generate metric billing outcomes #6066
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,12 @@ test-integration: build setup-venv ## run integration tests | |
| .venv/bin/pytest tests -n $(PYTEST_N) -v | ||
| .PHONY: test-integration | ||
|
|
||
| PYTEST_N ?= auto | ||
| TEST ?= tests | ||
| test-custom: build setup-venv ## run arbitrary tests | ||
| .venv/bin/pytest -n $(PYTEST_N) $(TEST) -vv | ||
| .PHONY: test-custom | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes to build systems etc. ideally are split out into separate PRs. Easier to review, also (which I don't expect to be a problem here), if we have to revert the PR it doesn't also revert these changes. |
||
|
|
||
| # Documentation | ||
|
|
||
| doc: doc-rust ## generate all API docs | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,6 +142,10 @@ pub enum Feature { | |
| #[serde(rename = "organizations:standalone-span-ingestion")] | ||
| DeprecatedStandaloneSpanIngestion, | ||
|
|
||
| /// Enable relay billing outcome generation. | ||
| #[serde(rename = "organizations:relay-generate-billing-outcome")] | ||
| GenerateBillingOutcome, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we would want this just under Also must admit that I am not sure if there is a functional difference these days between
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, ideally it's not in the deprecated group.
You can also roll out a |
||
|
|
||
| /// Forward compatibility. | ||
| #[doc(hidden)] | ||
| #[serde(other)] | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -58,6 +58,43 @@ impl MetricOutcomes { | |||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// Tracks billing-related outcomes for the list of buckets, adding the | ||||||||||||||||
| /// "billing_outcome_accepted" tag to the bucket if that bucket is accepted. | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Nit: |
||||||||||||||||
| pub fn track_billing_outcome(&self, scoping: Scoping, buckets: &mut [Bucket]) { | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this intentionally only implemented for spans?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, since this only serves the billing outcomes path.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you opted for this additional method because we need to temporarily add the tag to prevent double counting and longterm we can merge this into The comment in // Never emit accepted outcomes for surrogate metrics.
// These are handled from within Sentry.
if !matches!(outcome, Outcome::Accepted) {
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It felt right to keep them separate given the divergence in logic, at least for now. Comment updated.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
| let timestamp = Utc::now(); | ||||||||||||||||
| for bucket in buckets { | ||||||||||||||||
| let summary = bucket.summary(); | ||||||||||||||||
| match summary { | ||||||||||||||||
| BucketSummary::Spans { | ||||||||||||||||
| count, | ||||||||||||||||
| is_segment, | ||||||||||||||||
| was_transaction: _, | ||||||||||||||||
| } if count > 0 => { | ||||||||||||||||
| let all_categories = [DataCategory::Span, DataCategory::Transaction]; | ||||||||||||||||
| let num_categories = if is_segment { 2 } else { 1 }; | ||||||||||||||||
| let categories = &all_categories[0..num_categories]; | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| bucket | ||||||||||||||||
| .tags | ||||||||||||||||
| .insert("billing_outcome_accepted".to_owned(), "true".to_owned()); | ||||||||||||||||
|
|
||||||||||||||||
| for category in categories { | ||||||||||||||||
| self.outcomes.send(TrackOutcome { | ||||||||||||||||
| timestamp, | ||||||||||||||||
| scoping, | ||||||||||||||||
| outcome: Outcome::Accepted, | ||||||||||||||||
| event_id: None, | ||||||||||||||||
| remote_addr: None, | ||||||||||||||||
| category: *category, | ||||||||||||||||
| quantity: count as u32, | ||||||||||||||||
| }); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| _ => continue, | ||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd exhaustively match on |
||||||||||||||||
| }; | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// The return value of [`TrackableBucket::summary`]. | ||||||||||||||||
|
|
||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.