Skip to content

ref(mep): Move transaction thresholds to new tagging system, add histogram outliers [INGEST-541] [INGEST-947]#33722

Merged
untitaker merged 14 commits intomasterfrom
feat/metric-conditional-tagging-transaction-thresholds
Apr 21, 2022
Merged

ref(mep): Move transaction thresholds to new tagging system, add histogram outliers [INGEST-541] [INGEST-947]#33722
untitaker merged 14 commits intomasterfrom
feat/metric-conditional-tagging-transaction-thresholds

Conversation

@untitaker
Copy link
Member

In getsentry/relay#1225 we introduced a
general-purpose tagging system for transaction metrics. This may later
be extended:

  • we will add outlier tagging for histograms soon
  • session metrics could be tagged the same way (unlikely to happen for now, needs changes in Relay)

Here we move away from the purpose-built user satisfaction impl to the
more generic one

In getsentry/relay#1225 we introduced a
general-purpose tagging system for transaction metrics. This may later
be extended:

* we will add outlier tagging for histograms soon
* session metrics could be tagged the same way (unlikely to happen for now, needs changes in Relay)

Here we move away from the purpose-built user satisfaction impl to the
more generic one
@untitaker untitaker requested review from a team April 19, 2022 12:38
@untitaker untitaker requested a review from a team as a code owner April 19, 2022 12:38
@untitaker untitaker requested a review from a team April 19, 2022 12:38
@untitaker untitaker changed the title ref(mep): Move transaction thresholds to new tagging system ref(mep): Move transaction thresholds to new tagging system [INGEST-541] [INGEST-947] Apr 19, 2022
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Some minor remarks, otherwise LGTM!

"inner": [
{
"op": "gt",
"name": _TRANSACTION_METRICS_TO_RULE_FIELD[threshold.metric],
Copy link
Member Author

Choose a reason for hiding this comment

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

@untitaker untitaker changed the title ref(mep): Move transaction thresholds to new tagging system [INGEST-541] [INGEST-947] ref(mep): Move transaction thresholds to new tagging system, add histogram outliers [INGEST-541] [INGEST-947] Apr 19, 2022
@untitaker untitaker requested a review from jjbayer April 19, 2022 18:53
@untitaker
Copy link
Member Author

@jjbayer making you re-review this since I added histogram outliers

"inner": [
{"op": "eq", "name": "event.contexts.trace.op", "value": op},
{"op": "eq", "name": "event.platform", "value": platform},
{"op": "gte", "name": "event.duration", "value": p25 + 3 * p75},
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a comment here on why we use p25 + 3 * p75? I tried googling Tukey's fences but the only formula I found was p75 + 3 * (p75 - p25).

Copy link
Member Author

Choose a reason for hiding this comment

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

oof you're right, the calculation is completely messed up. copied it from a document that is apparently wrong

"condition": {
"op": "and",
"inner": [
{"op": "gte", "name": "event.duration", "value": 0},
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that any (platform, op) combination that is not among the Top-50 gets tagged as 100% outliers?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup this was also wrong. now changed to inlier

@untitaker untitaker merged commit 2a9bce8 into master Apr 21, 2022
@untitaker untitaker deleted the feat/metric-conditional-tagging-transaction-thresholds branch April 21, 2022 13:20
@github-actions github-actions bot locked and limited conversation to collaborators May 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants