Skip to content

Conversation

@jjbayer
Copy link
Member

@jjbayer jjbayer commented Dec 7, 2023

Performance score measurements are special measurements derived by relay. As such, they do not require the full set of metric tags that SDK-side measurements get.

Limiting the number of tags will reduce cardinality by ~20%, according to my analysis.

#skip-changelog

Comment on lines 693 to 697
"browser.name": "Chrome",
"environment": "fake_environment",
"release": "1.2.3",
"transaction": "gEt /api/:version/users/",
"transaction.op": "mYOp",
Copy link
Member Author

Choose a reason for hiding this comment

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

Without the tag filter, this would include platform, dist, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

@edwardgou-sentry @k-fish apart from the performance score UI, could there be a more generic measurements-related part of the UI that breaks if these metrics come with only a subset of tags?

Copy link
Contributor

@edwardgou-sentry edwardgou-sentry Dec 7, 2023

Choose a reason for hiding this comment

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

As far as I know, Dashboards is the only other place users would be able to write a query for Performance Scores using metrics. But I'm pretty sure that if a metrics query breaks due to missing tags, it will fall back to the indexed dataset instead. I think this behaviour should be ok

Comment on lines +299 to +303
let is_performance_score = name == "score.total"
|| name
.strip_prefix("score.weight.")
.or_else(|| name.strip_prefix("score."))
.map_or(false, |suffix| measurement_names.contains(suffix));
Copy link
Member Author

Choose a reason for hiding this comment

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

This is clumsy: It would be better to mark measurements as "performance score" somehow, but they might originate from a downstream relay so any kind of flag would have to be serialized into the event JSON.

@jjbayer jjbayer marked this pull request as ready for review December 7, 2023 13:54
@jjbayer jjbayer requested a review from a team as a code owner December 7, 2023 13:54
Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

👍 looks good. On top of limiting tags, I also mentioned in my original doc that it's possible we don't store distributions for perf score since it just needs sum + count (avg), which are both counters, which could drastically lower storage.

@jjbayer jjbayer merged commit 4802ec5 into master Dec 11, 2023
@jjbayer jjbayer deleted the feat/perf-score-tags branch December 11, 2023 08:00
@jjbayer
Copy link
Member Author

jjbayer commented Dec 11, 2023

it's possible we don't store distributions for perf score since it just needs sum + count (avg), which are both counters, which could drastically lower storage.

@k-fish good point. Could we build this feature by just using the "counter" metric type (which supports floats)?

@k-fish
Copy link
Member

k-fish commented Dec 11, 2023

@jjbayer yes, it could be count (sum) and count (count) then divide them.

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