ref(spans): Reduce number of tags on span metrics #2907

jjbayer · 2024-01-04T09:42:04Z

During the development of Starfish we've repeatedly added new tags to span metrics without restricting the conditions under which a tag is set (that is, what queries actually require a tag).

This PR introduces a new config for the exclusive_time and exclusive_time_light metrics, which limits the number of cases in which the metrics and / or their tags are extracted. Some tags are removed entirely.

For now, the new config will only be enabled for projects that have the all-modules feature flag enabled, i.e. sentry-internal. After we've verified that the new config covers all required cases, we can enable the new config for all orgs.

ref: https://github.com/getsentry/team-ingest/issues/252

…-module

jjbayer · 2024-01-04T14:43:03Z

tests/integration/test_store.py

-        {
-            "org_id": 1,
-            "project_id": 42,
-            "name": "d:spans/exclusive_time@millisecond",
-            "type": "d",
-            "value": [500.0, 1500],
-            "timestamp": expected_timestamp,
-            "tags": {"span.status": "ok", "span.op": "default"},
-            "retention_days": 90,
-        },


No exclusive time for op:default.

jjbayer · 2024-01-04T15:08:12Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

-    Bucket {
-        timestamp: UnixTimestamp(1597976302),
-        width: 0,
-        name: "d:spans/exclusive_time@millisecond",
-        value: Distribution(
-            [
-                2000.0,
-            ],
-        ),
-        tags: {
-            "environment": "fake_environment",
-            "span.action": "GET",
-            "span.category": "http",
-            "span.description": "GET http://domain.tld",
-            "span.domain": "domain.tld",
-            "span.group": "d9dc18637d441612",
-            "span.op": "http.client",
-            "transaction": "gEt /api/:version/users/",
-            "transaction.method": "POST",
-            "transaction.op": "myop",
-        },
-    },
-    Bucket {
-        timestamp: UnixTimestamp(1597976302),
-        width: 0,
-        name: "d:spans/exclusive_time_light@millisecond",
-        value: Distribution(
-            [
-                2000.0,
-            ],
-        ),
-        tags: {
-            "environment": "fake_environment",
-            "span.action": "GET",
-            "span.category": "http",
-            "span.description": "GET http://domain.tld",
-            "span.domain": "domain.tld",
-            "span.group": "d9dc18637d441612",
-            "span.op": "http.client",
-            "transaction.method": "POST",
-            "transaction.op": "myop",
-        },
-    },


@shruthilayaj @gggritso no more exclusive time metrics for http.client, except on mobile screen load spans (see test_extract_span_metrics_mobile_screen).

Is there any other use case where we still need http.client spans?

Not that I know of, but it'll come up immediately when we work on an API module (unsure when, if ever)

jjbayer · 2024-01-04T15:10:29Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

@@ -637,11 +152,8 @@ expression: metrics
            "span.domain": "table",
            "span.group": "f4a7fef06db3d88e",
            "span.op": "db.sql.query",
-            "span.status": "ok",


@gggritso span.status will be removed for db spans.

jjbayer · 2024-01-04T15:11:38Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

@@ -637,11 +152,8 @@ expression: metrics
            "span.domain": "table",
            "span.group": "f4a7fef06db3d88e",
            "span.op": "db.sql.query",
-            "span.status": "ok",
-            "span.system": "postgresql",


@gggritso we still use span.system in Relay to pick an SQL parser, but it won't be a metric tag anymore.

👍🏻 makes sense! Though, caveat: there's a chance we'll need this back in the future, when we come back to do a database server selector. Right now we're planning to use the database host (doesn't exist as a tag yet) but we might need the system, too

Ack, brining it back should not be a problem!

jjbayer · 2024-01-04T15:12:02Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

            "transaction": "gEt /api/:version/users/",
            "transaction.method": "POST",
-            "transaction.op": "myop",


@gggritso transaction.op will be removed for db spans.

I think this is fine for now, but it's a little risky! When we get back to working on a Web Service View (which hopefully is soon, but I don't know!) we'll probably want to filter time spent to http.server spans vs. task spans. Again, this is just me speculating, but still!

If we can live with not having backdated data at the point where we start developing that view, I would drop it for now and bring it back when it's needed.

jjbayer · 2024-01-04T15:14:11Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

@@ -661,10 +173,6 @@ expression: metrics
            "span.domain": "table",
            "span.group": "f4a7fef06db3d88e",
            "span.op": "db.sql.query",
-            "span.status": "ok",
-            "span.system": "postgresql",
-            "transaction.method": "POST",


@gggritso transaction.method is still a tag on exclusive_time, but not on exclusive_time_light.

Makes sense, I don't know of any product use-case for querying by transaction method but no transaction

jjbayer · 2024-01-04T15:15:47Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

-    Bucket {
-        timestamp: UnixTimestamp(1597976302),
-        width: 0,
-        name: "d:spans/exclusive_time@millisecond",
-        value: Distribution(
-            [
-                2000.0,
-            ],
-        ),
-        tags: {
-            "environment": "fake_environment",
-            "span.category": "cache",
-            "span.description": "GET *",
-            "span.group": "37e3d9fab1ae9162",
-            "span.op": "cache.get_item",
-            "span.status": "ok",
-            "transaction": "gEt /api/:version/users/",
-            "transaction.method": "POST",
-            "transaction.op": "myop",
-        },
-    },
-    Bucket {
-        timestamp: UnixTimestamp(1597976302),
-        width: 0,
-        name: "d:spans/exclusive_time_light@millisecond",
-        value: Distribution(
-            [
-                2000.0,
-            ],
-        ),
-        tags: {
-            "environment": "fake_environment",
-            "span.category": "cache",
-            "span.description": "GET *",
-            "span.group": "37e3d9fab1ae9162",
-            "span.op": "cache.get_item",
-            "span.status": "ok",
-            "transaction.method": "POST",
-            "transaction.op": "myop",
-        },
-    },


@gggritso no more exclusive time metrics for cache spans.

Make sense! Same caveat about Web Service view, we'll probably want cache spans then

jjbayer · 2024-01-04T15:16:29Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

-            "span.category": "db",
-            "span.description": "GET *",
-            "span.group": "37e3d9fab1ae9162",
-            "span.op": "db.redis",


@gggritso no more exclusive time metrics for redis spans (for now).

Make sense! Same caveat about Web Service view, we'll probably want redis spans then

jjbayer · 2024-01-04T15:19:13Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

            "transaction": "gEt /api/:version/users/",
-            "transaction.method": "POST",
-            "transaction.op": "myop",


@DominikB2014 span.domain, span.status, transaction.method and transaction.op will disappear for resource spans.

@jjbayer We use span.domain for the domain dropdown, will there be an alternative?

Whoops. how did I miss that. Will add it back.

🙏 Thanks @jjbayer!

jjbayer · 2024-01-04T15:20:23Z

...n/snapshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_mobile.snap

@@ -218,7 +221,6 @@ expression: "(&event.value().unwrap().spans, metrics)"
                "os.name": "iOS",
                "release": "1.2.3",
                "span.op": "ui.load.initial_display",
-                "transaction.method": "GET",


@shruthilayaj transaction.method will disappear for mobile span metrics.

…-module

olksdr · 2024-01-08T11:13:09Z

relay-dynamic-config/src/defaults.rs

-    // Add conditions to filter spans if a specific module is enabled.
-    // By default, this will extract all spans.
-    let (span_op_conditions, resource_condition) = if project_config
+    // "all-modules" is our de-facto feature flag for experimental features.


Suggested change

// "all-modules" is our de-facto feature flag for experimental features.

// The "all-modules" is our de-facto feature flag for experimental features.

jjbayer · 2024-01-05T09:49:41Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

@@ -637,11 +152,8 @@ expression: metrics
            "span.domain": "table",
            "span.group": "f4a7fef06db3d88e",
            "span.op": "db.sql.query",
-            "span.status": "ok",
-            "span.system": "postgresql",


Ack, brining it back should not be a problem!

jjbayer · 2024-01-05T09:51:28Z

...pshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_all_modules.snap

            "transaction": "gEt /api/:version/users/",
            "transaction.method": "POST",
-            "transaction.op": "myop",


If we can live with not having backdated data at the point where we start developing that view, I would drop it for now and bring it back when it's needed.

relay-dynamic-config/src/defaults.rs

Follow-up to #2907: Apply reduced set of span metrics and tags regardless of feature flag. ref: getsentry/team-ingest#252

jjbayer added 4 commits January 2, 2024 15:43

rename

40d0439

first pass

9fbd42f

feat: only for all-modules

bfcdd4f

test

310dc5e

jjbayer changed the title ~~Ref/span metrics by module~~ ref(spans): Reduce number of tags on span metrics Jan 4, 2024

jjbayer added 7 commits January 4, 2024 14:06

test

42fe391

comments

e159feb

fix: category condition

35bc1ea

test: snapshot

e7485a1

Merge remote-tracking branch 'origin/master' into ref/span-metrics-by…

e2c138b

…-module

test: screen

e5e1fae

fix: test

b74d0ae

jjbayer requested a review from phacops January 4, 2024 16:03

jjbayer marked this pull request as ready for review January 4, 2024 16:32

jjbayer requested a review from a team as a code owner January 4, 2024 16:32

jjbayer commented Jan 4, 2024

View reviewed changes

jjbayer added 2 commits January 5, 2024 09:44

Merge remote-tracking branch 'origin/master' into ref/span-metrics-by…

696f453

…-module

doc: changelog

635a7cf

olksdr approved these changes Jan 8, 2024

View reviewed changes

jjbayer commented Jan 8, 2024

View reviewed changes

Update relay-dynamic-config/src/defaults.rs

d280423

jjbayer enabled auto-merge (squash) January 8, 2024 11:19

iker-barriocanal approved these changes Jan 8, 2024

View reviewed changes

jjbayer merged commit f7aea71 into master Jan 8, 2024
20 checks passed

jjbayer deleted the ref/span-metrics-by-module branch January 8, 2024 11:39

jjbayer mentioned this pull request Jan 9, 2024

ref(spans): Reduced set of metrics/tags for all orgs #2924

Merged

jjbayer added a commit that referenced this pull request Jan 10, 2024

ref(spans): Reduced set of metrics/tags for all orgs (#2924)

4bccbbb

Follow-up to #2907: Apply reduced set of span metrics and tags regardless of feature flag. ref: getsentry/team-ingest#252

jjbayer mentioned this pull request Feb 15, 2024

fix(spans): Ingest span metrics for http spans #3111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(spans): Reduce number of tags on span metrics #2907

ref(spans): Reduce number of tags on span metrics #2907

jjbayer commented Jan 4, 2024 •

edited

Loading

jjbayer Jan 4, 2024

jjbayer Jan 4, 2024

gggritso Jan 4, 2024

jjbayer Jan 4, 2024

gggritso Jan 4, 2024

jjbayer Jan 4, 2024

gggritso Jan 4, 2024

jjbayer Jan 5, 2024

jjbayer Jan 4, 2024

gggritso Jan 4, 2024 •

edited

Loading

jjbayer Jan 5, 2024

gggritso Jan 8, 2024

jjbayer Jan 4, 2024

gggritso Jan 4, 2024

jjbayer Jan 4, 2024

gggritso Jan 4, 2024

jjbayer Jan 4, 2024

gggritso Jan 4, 2024

jjbayer Jan 4, 2024

DominikB2014 Jan 8, 2024

jjbayer Jan 8, 2024

DominikB2014 Jan 8, 2024

jjbayer Jan 4, 2024

olksdr Jan 8, 2024

jjbayer Jan 5, 2024

jjbayer Jan 5, 2024

	// "all-modules" is our de-facto feature flag for experimental features.
	// The "all-modules" is our de-facto feature flag for experimental features.

ref(spans): Reduce number of tags on span metrics #2907

ref(spans): Reduce number of tags on span metrics #2907

Conversation

jjbayer commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gggritso Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer commented Jan 4, 2024 •

edited

Loading

gggritso Jan 4, 2024 •

edited

Loading