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(metrics): first custom metric sent signal #71096

Merged
merged 3 commits into from
May 22, 2024

Conversation

obostjancic
Copy link
Member

@obostjancic obostjancic commented May 17, 2024

@obostjancic obostjancic requested a review from a team May 17, 2024 09:34
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 17, 2024
Copy link
Member

@matejminar matejminar left a comment

Choose a reason for hiding this comment

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

Do we need to register this event like this?

from sentry import analytics

@matejminar
Copy link
Member

matejminar commented May 17, 2024

Also, that event is in amplitude visible as "Sent First Replay". When I searched sentry repos, I found this mapping:
https://github.com/getsentry/etl/blob/9e765d08e1a85e0697e2655c7e7d6c330b6d7b3c/workspace/dags/sql/analytics/amplitude-analytics-unagg.sql
We probably need to do something like that as well.

+this?
https://github.com/getsentry/etl/blob/9e765d08e1a85e0697e2655c7e7d6c330b6d7b3c/workspace/dags/sql/config/amplitude_analytics_config.py#L19

@obostjancic obostjancic marked this pull request as ready for review May 21, 2024 13:41
@obostjancic obostjancic requested a review from a team as a code owner May 21, 2024 13:41
@obostjancic
Copy link
Member Author

Also, that event is in amplitude visible as "Sent First Replay". When I searched sentry repos, I found this mapping: getsentry/etl@9e765d0/workspace/dags/sql/analytics/amplitude-analytics-unagg.sql We probably need to do something like that as well.

+this? getsentry/etl@9e765d0/workspace/dags/sql/config/amplitude_analytics_config.py#L19

👍
Done in fe7472e and https://github.com/getsentry/etl/pull/1441

Copy link

codecov bot commented May 21, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 77.87%. Comparing base (6ebdfa3) to head (def932a).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #71096      +/-   ##
==========================================
- Coverage   77.87%   77.87%   -0.01%     
==========================================
  Files        6529     6529              
  Lines      290909   290912       +3     
  Branches    50338    50339       +1     
==========================================
- Hits       226550   226544       -6     
- Misses      58120    58125       +5     
- Partials     6239     6243       +4     
Files Coverage Δ
src/sentry/ingest/billing_metrics_consumer.py 87.50% <100.00%> (-1.08%) ⬇️
src/sentry/receivers/onboarding.py 80.93% <41.66%> (-2.11%) ⬇️

... and 6 files with indirect coverage changes


# We assume that the flag update is reflected in the cache, so that upcoming calls will get the up-to-
# date project with the `has_custom_metrics` flag set to true.
project.update(flags=F("flags").bitor(Project.flags.has_custom_metrics))
Copy link
Member

Choose a reason for hiding this comment

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

If we don't update this flag anymore, we will run the amplitude event for every incoming custom metric of a project that never set that flag before. Do we want that?

Copy link
Member

Choose a reason for hiding this comment

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

Missed the other code, nvm.

@iambriccardo iambriccardo self-requested a review May 22, 2024 11:10
@@ -318,6 +320,31 @@ def record_first_cron_checkin(project, monitor_id, **kwargs):
)


@first_custom_metric_received.connect(weak=False)
def record_first_custom_metric(project, **kwargs):
project.update(flags=F("flags").bitor(Project.flags.has_custom_metrics))
Copy link
Member Author

Choose a reason for hiding this comment

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

@iambriccardo we update the flag here

@obostjancic obostjancic merged commit 2fbb025 into master May 22, 2024
49 of 50 checks passed
@obostjancic obostjancic deleted the ogi/feat/metrics-fist-custom-sent branch May 22, 2024 12:08
Copy link

sentry-io bot commented May 22, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ KeyError: 'first_custom_metric.sent' sentry.analytics.event_manager in get View Issue

Did you find this useful? React with a 👍 or 👎

obostjancic added a commit that referenced this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace first custom metric sent event with amplitude
4 participants