Skip to content
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

feat(escalating-v2): Emit escalating metrics #52774

Merged
merged 5 commits into from
Jul 20, 2023
Merged

Conversation

snigdhas
Copy link
Member

Update the ESCALATING_ISSUES use case during post_process as we see new events come in.

@snigdhas snigdhas added this to the Escalating Issues V2 (EA) milestone Jul 12, 2023
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 12, 2023
org_id=event.project.organization_id,
project_id=event.project.id,
metric_name="event_ingested",
value=1,
Copy link
Member Author

@snigdhas snigdhas Jul 12, 2023

Choose a reason for hiding this comment

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

@ayirr7 this should get aggregated as part of the counter right?

Copy link
Member

Choose a reason for hiding this comment

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

yes, that's correct

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #52774 (c0ef368) into master (9ef3ba5) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #52774      +/-   ##
==========================================
+ Coverage   79.48%   79.50%   +0.02%     
==========================================
  Files        4935     4940       +5     
  Lines      207668   208164     +496     
  Branches    35453    35507      +54     
==========================================
+ Hits       165069   165505     +436     
- Misses      37570    37621      +51     
- Partials     5029     5038       +9     
Impacted Files Coverage Δ
src/sentry/tasks/post_process.py 90.35% <100.00%> (+0.12%) ⬆️

... and 200 files with indirect coverage changes

@snigdhas snigdhas marked this pull request as ready for review July 13, 2023 17:43
@snigdhas snigdhas requested a review from a team as a code owner July 13, 2023 17:43
metric_name="event_ingested",
value=1,
tags={"group": event.group_id},
unit=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

what is unit?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's the mri_unit in this template ":/@". I've mostly seen units of time (millisecond, second) here

Copy link
Contributor

Choose a reason for hiding this comment

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

@ayirr7 what should this be set to for our needs?

Copy link
Member

Choose a reason for hiding this comment

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

The unit for the metric you are emitting (whatever you are measuring). If there is no unit then None is fine

project_id=event.project.id,
metric_name="event_ingested",
value=1,
tags={"group": event.group_id},
Copy link
Member

Choose a reason for hiding this comment

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

Is event.group_id a string?

Copy link
Member

Choose a reason for hiding this comment

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

Tag keys and values must both be strings to be compatible with the Kafka schema here https://github.com/getsentry/sentry-kafka-schemas/blob/main/schemas/ingest-metrics.v1.schema.json

@snigdhas snigdhas merged commit 11e749f into master Jul 20, 2023
56 checks passed
@snigdhas snigdhas deleted the snigdha/metrics-emit branch July 20, 2023 18:17
@beezz beezz added the Trigger: Revert add to a merged PR to revert it (skips CI) label Jul 21, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: 89889c3

getsentry-bot added a commit that referenced this pull request Jul 21, 2023
@@ -16,6 +16,8 @@
from sentry.issues.grouptype import GroupCategory
from sentry.issues.issue_occurrence import IssueOccurrence
from sentry.killswitches import killswitch_matches_context
from sentry.sentry_metrics.kafka import KafkaMetricsBackend
Copy link
Contributor

@onewland onewland Jul 21, 2023

Choose a reason for hiding this comment

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

@ayirr7 I think we should hide which backend gets used from the caller.

it should be something like

from sentry.sentry_metrics import get_sentry_metrics_backend

# today this returns a KafkaMetricsBackend, but later it
# should be configurable from somewhere outside the importing file
metrics_backend = get_sentry_metrics_backend() 

This also makes it easier to only have one connection (pool) per process

Copy link
Member

Choose a reason for hiding this comment

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

Sentry has the service delegator / LazyServiceWrapper abstraction which is commonly used for this

@github-actions github-actions bot locked and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants