feat(metrics): Count transactions toward root project [TET-627] by jjbayer · Pull Request #1734 · getsentry/relay

jjbayer · 2023-01-10T12:23:00Z

This PR introduce new metric count_per_root_project, similar to duration,
but we associate it with root_project id and store as sampling_metrics vector (similar name we do in another places).

So we can use it in outcomes to know for example:

Frontend generates 8M (direct) + 120M (backend), so count_per_root_project = 128M for dependent projects.

or fallback to count(duration) == count_per_root_project for independent projects.

The metric is tagged with the transaction name from the trace header, which should be low cardinality (SDKs should not set it when transaction source is url).

TODO:

Make it compile
Fix rust tests
Fix python integration tests

related PR getsentry/sentry#43167

untitaker · 2023-01-11T15:41:59Z

relay-server/src/actors/store.rs

    #[serde(flatten)]
    value: BucketValue,
    timestamp: UnixTimestamp,
-    #[serde(skip_serializing_if = "BTreeMap::is_empty")]


i am reviewing this change only. i think we can revisit the schema we actually want later, but as pointed out in private conversations, the metrics indexer currently does not handle the absence of tags correctly.

jjbayer

The PR looks good to me. Let's make sure Search and Storage is aware that we are adding a counter metric before we deploy this.

relay-server/src/metrics_extraction/transactions.rs

jjbayer · 2023-01-31T11:41:36Z

relay-server/src/actors/processor.rs

                        // requires recomputation of the context.
                        state.envelope_context.update(&state.envelope);

+                        let has_metrics = state.extracted_metrics.project_metrics.is_empty();


Suggested change

let has_metrics = state.extracted_metrics.project_metrics.is_empty();

let has_metrics = !state.extracted_metrics.project_metrics.is_empty();

I am surprised that no test case caught this.

jjbayer · 2023-01-31T11:53:27Z

tests/integration/test_metrics.py


    assert metrics.keys() == {
        "d:transactions/duration@millisecond",
+        "c:transactions/count_per_root_project@none",


As far as I can tell, these test cases all add this metric to current project, because there is no DSC involved. Please also add tests for the following:

a test with a DSC where the DSC's project ID is different from the event's project ID.

a test where we assert that the DSC's transaction field is used as a tag on the metric.

relay-server/src/actors/processor.rs

olksdr · 2023-02-01T08:06:54Z

CHANGELOG.md

 - Add error and sample rate fields to the replay event parser. ([#1745](https://github.com/getsentry/relay/pull/1745))
 - Add `instruction_addr_adjustment` field to `RawStacktrace`. ([#1716](https://github.com/getsentry/relay/pull/1716))
 - Add SSL support to `relay-redis` crate. It is possible to use `rediss` scheme to connnect to Redis cluster using TLS. ([#1772](https://github.com/getsentry/relay/pull/1772))
+- Add count transactions toward root project. ([#1734](https://github.com/getsentry/relay/pull/1734))


Please, move this to Unreleased section.

@olksdr done.

olksdr · 2023-02-01T08:19:42Z

relay-server/src/actors/processor.rs

    fn process_sessions(&self, state: &mut ProcessEnvelopeState) {
        let received = state.envelope_context.received_at();
-        let extracted_metrics = &mut state.extracted_metrics;
+        let extracted_metrics = &mut &mut state.extracted_metrics.project_metrics;


Doesn't this work just with taking one mutable reference?

Suggested change

let extracted_metrics = &mut &mut state.extracted_metrics.project_metrics;

let extracted_metrics = &mut state.extracted_metrics.project_metrics;

good catch! yeah it works without second &mut

jjbayer · 2023-02-01T10:43:26Z

relay-server/src/metrics_extraction/transactions.rs

-    metrics: &mut Vec<Metric>, // output parameter
+    metrics: &mut Vec<Metric>,          // output parameter
+    sampling_metrics: &mut Vec<Metric>, // output parameter
+    transaction_from_dsc: Option<&str>,


nit: Because metrics and sampling_metrics are output parameters, I would put them at the end of the argument list.

fixed afa6576

relay-server/src/metrics_extraction/transactions.rs

jjbayer · 2023-02-01T11:00:44Z

tests/integration/test_metrics.py

+        "name": "c:transactions/count_per_root_project@none",
+        "type": "c",
+        "value": 3.0,
+    }


Isn't this use case already tested by test_transaction_metrics? Can we alter this test so that this metric is emitted with project_id: 41, where 41 is the root project. We could alter the signature of send_transaction to accept an explicit trace header:

relay.send_transaction(42, transaction, trace_header={"public_key": public_key_of_project_41, "transaction": "root_transaction"})

It would also be good to have one test case where count_per_root_project is emitted without a transaction tag (maybe I missed it).

agree with you suggestion,

regarding:

It would also be good to have one test case where count_per_root_project is emitted without a transaction tag (maybe I missed it).

I think we cover it separately in other tests where we can see tags = {}, but I can write separate test for that.

Thanks, no need to add a separate test if it's already covered.

jjbayer · 2023-02-02T08:19:09Z

tests/integration/test_metrics.py

    return metrics


+def metrics_by_name_group_by_project(metrics_consumer, count, timeout=None):


This helper function does not seem to be using count at all. I feel like this function could call metrics_by_name internally, or vice versa?

count is huge antipattern IMO, I'll remove it from args :)

let me explain, this helper has small problem:

def metrics_by_name(metrics_consumer, count, timeout=None): metrics = {} for _ in range(count): metric = metrics_consumer.get_metric(timeout) metrics[metric["name"]] = metric metrics_consumer.assert_empty() return metrics

if you have 2 projects and 2 metrics with the same name you will see only the last one, so that's why I decided to create a new helper function.

And the second problem, you need manually to set the count - I prefer to get all metrics - since we have this assert_empty and then return data.

jjbayer · 2023-02-02T08:21:51Z

tests/integration/test_metrics.py

+        "name": "c:transactions/count_per_root_project@none",
+        "type": "c",
+        "value": 3.0,
+    }


Thanks, no need to add a separate test if it's already covered.

relay-server/src/metrics_extraction/transactions.rs

jjbayer · 2023-02-02T08:30:14Z

tests/integration/test_metrics.py

+        "foo": {"value": 1.2},
+        "bar": {"value": 1.3},
+    }
+    relay.send_transaction(41, transaction, transaction_from_dsc="test")


Instead of sending this transaction to project 41, I would send it to project 42 with project_id=41 in the DSC. And then verify that the metric has project 41, not 42. This is what I meant originally with

a test with a DSC where the DSC's project ID is different from the event's project ID.

jjbayer

I approve this PR. Please notify SnS when this change goes live, so they can observe the increase in stored metrics / tags.

iambriccardo

LGTM

olksdr

lgtm

* master: feat(metrics): Count transactions toward root project [TET-627] (#1734)

#42939) This PR implements prioritize by project bias. In detail: We run celery task every 24 at 8:00AM (UTC randomly selected) for every ORG (we call it *prioritise by project snuba query* ) and all projects inside this org, and for a given combination of org and projects run an adjustment model to recalculate sample rates if necessary. Then we cache sample rate using redis cluster -> `SENTRY_DYNAMIC_SAMPLING_RULES_REDIS_CLUSTER` using this pattern for key: `f"ds::o:{org_id}:p:{project_id}:prioritise_projects"`. When relay fetches `projectconfig` endpoint we run `generate_rules` functions to generate all dynamic sampling biases, so and we check if we have adjusted sample rate for this project in the cache, so we apply it as **uniform bias**, otherwise we use default one. Regarding *prioritize by project snuba query* is cross org snuba query that utilizes a new generic counter metric, which was introduced in [relay]( getsentry/relay#1734) `c:transactions/count_per_root_project@none`. TODO: - [x] Provision infrastructure to run clickhouse clusters for the counters tables. This is primarily dependent on ops - [x] Start running the snuba consumers to read and write to the counters table. SnS can work on this - [x] Add unit-tests; - [x] Update snuba query using new metric - [x] Hide behind feature flag related PRs: - Implement new metric in relay: getsentry/relay#1734 - Add org generic counters [TET-695] getsentry/snuba#3708 - Introduce new storages for counters in snuba getsentry/snuba#3679 - Add feature flag: https://github.com/getsentry/getsentry/pull/9323 - Add cross organization methods for the string indexer #45076 #45076 [TET-695]: https://getsentry.atlassian.net/browse/TET-695?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com> Co-authored-by: Nar Saynorath <nar.saynorath@sentry.io>

andriisoldatenko changed the title ~~feat(metrics): Count transactions toward root project~~ feat(metrics): Count transactions toward root project [TET-627] Jan 10, 2023

andriisoldatenko mentioned this pull request Jan 10, 2023

feat(dynamic-sampling): Implement prioritize by project bias [TET-574] getsentry/sentry#42939

Merged

5 tasks

andriisoldatenko marked this pull request as ready for review January 11, 2023 15:33

andriisoldatenko requested review from a team and untitaker January 11, 2023 15:33

untitaker reviewed Jan 11, 2023

View reviewed changes

andriisoldatenko requested review from jan-auer, olksdr and untitaker January 12, 2023 11:36

jjbayer commented Jan 12, 2023

View reviewed changes

andriisoldatenko reviewed Jan 13, 2023

View reviewed changes

relay-server/src/metrics_extraction/transactions.rs Show resolved Hide resolved

andriisoldatenko force-pushed the feat/metrics-count-tx-for-root branch from c932995 to c727b43 Compare January 16, 2023 08:35

andriisoldatenko self-requested a review January 16, 2023 12:25

andriisoldatenko approved these changes Jan 18, 2023

View reviewed changes

andriisoldatenko force-pushed the feat/metrics-count-tx-for-root branch from 63432ac to 0849c6e Compare January 26, 2023 10:05

jjbayer and others added 14 commits January 30, 2023 13:21

wip

9349537

wip

756defd

fixup!

b07ab4a

fixup!

47e0bba

fix clippy

9f87b34

refactor a bit

aa2edab

adjust integration tests

0b82254

fix the tests

d38022c

fix the tests

e7f90b6

fixup!

e6891a9

fixup!

ae315a4

update changelog

0029076

remove flakiness by sorting results

32db4a9

fix clippy

ddcb598

andriisoldatenko force-pushed the feat/metrics-count-tx-for-root branch from 0849c6e to ddcb598 Compare January 30, 2023 12:25

Andrii Soldatenko added 2 commits January 30, 2023 15:15

add tags to count_root_project metric

af60a60

add tags to count_root_project metric

c0dbf5c

andriisoldatenko requested a review from iambriccardo January 31, 2023 09:04

Merge branch 'master' into feat/metrics-count-tx-for-root

8638cdd

jjbayer commented Jan 31, 2023

View reviewed changes

jjbayer assigned andriisoldatenko Jan 31, 2023

andriisoldatenko reviewed Jan 31, 2023

View reviewed changes

relay-server/src/actors/processor.rs Show resolved Hide resolved

Andrii Soldatenko added 3 commits January 31, 2023 16:27

update tests

801fabd

Merge branch 'master' into feat/metrics-count-tx-for-root

a16998e

add more tests

bfd8f93

olksdr reviewed Feb 1, 2023

View reviewed changes

Andrii Soldatenko added 2 commits February 1, 2023 09:29

move changes in CHANGELOG.md to unreleased section

a8bfd98

remove second &mut

969596c

jjbayer commented Feb 1, 2023

View reviewed changes

fixup!

c241207

jjbayer commented Feb 2, 2023

View reviewed changes

Andrii Soldatenko added 2 commits February 2, 2023 09:33

fixup!

e762156

refactor test

4a8a7e2

andriisoldatenko requested a review from olksdr February 2, 2023 09:45

refactor test

afa6576

jjbayer commented Feb 2, 2023

View reviewed changes

Merge branch 'master' into feat/metrics-count-tx-for-root

7ca4b94

iambriccardo approved these changes Feb 2, 2023

View reviewed changes

olksdr reviewed Feb 2, 2023

View reviewed changes

olksdr approved these changes Feb 2, 2023

View reviewed changes

jjbayer merged commit 57aacf0 into master Feb 2, 2023

jjbayer deleted the feat/metrics-count-tx-for-root branch February 2, 2023 12:46

jan-auer added a commit that referenced this pull request Feb 2, 2023

Merge branch 'master' into replays-use-user-configured-scrubbers

9675b47

* master: feat(metrics): Count transactions toward root project [TET-627] (#1734)

	let has_metrics = state.extracted_metrics.project_metrics.is_empty();
	let has_metrics = !state.extracted_metrics.project_metrics.is_empty();

	let extracted_metrics = &mut &mut state.extracted_metrics.project_metrics;
	let extracted_metrics = &mut state.extracted_metrics.project_metrics;

		return metrics


		def metrics_by_name_group_by_project(metrics_consumer, count, timeout=None):

Conversation

jjbayer commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjbayer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andriisoldatenko Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjbayer left a comment

Choose a reason for hiding this comment

Uh oh!

iambriccardo left a comment

Choose a reason for hiding this comment

Uh oh!

olksdr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jjbayer commented Jan 10, 2023 •

edited

Loading

andriisoldatenko Feb 1, 2023 •

edited

Loading