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.

Sign up for GitHub

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

Jump to bottom

feat(release-health): Sessions should not be default #67353

Merged
merged 16 commits into from Apr 3, 2024
Merged

feat(release-health): Sessions should not be default #67353

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
804e0e4
feat: Sessions should not be default
lynnagara Mar 20, 2024
bea49a7
requires metrics
lynnagara Mar 20, 2024
ca558be
fix some tests
lynnagara Mar 20, 2024
482595b
Merge branch 'master' into sessions-is-not-default
lynnagara Apr 1, 2024
ef22bc4
update tests
lynnagara Apr 1, 2024
b88e787
more test stuff
lynnagara Apr 1, 2024
d6184ea
yet more tests
lynnagara Apr 1, 2024
c86e019
one more
lynnagara Apr 1, 2024
5dabab4
Fix offset missing
iambriccardo Apr 2, 2024
1ef5c0d
Remove xfail
iambriccardo Apr 2, 2024
16142cf
Try fix
iambriccardo Apr 2, 2024
62855c0
Try fix
iambriccardo Apr 2, 2024
55401ea
Try fix
iambriccardo Apr 2, 2024
929c1fa
Fix
iambriccardo Apr 3, 2024
7315406
Fix
iambriccardo Apr 3, 2024
42ac0cc
xfail test
iambriccardo Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/sentry/conf/server.py
View file
Open in desktop
Expand Up @@ -2331,13 +2331,11 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]:
SENTRY_METRICS_INDEXER_ENABLE_SLICED_PRODUCER = False

# Release Health
SENTRY_RELEASE_HEALTH = "sentry.release_health.sessions.SessionsReleaseHealthBackend"
SENTRY_RELEASE_HEALTH = "sentry.release_health.metrics.MetricsReleaseHealthBackend"
SENTRY_RELEASE_HEALTH_OPTIONS: dict[str, Any] = {}

# Release Monitor
SENTRY_RELEASE_MONITOR = (
"sentry.release_health.release_monitor.sessions.SessionReleaseMonitorBackend"
)
SENTRY_RELEASE_MONITOR = "sentry.release_health.release_monitor.metrics.MetricReleaseMonitorBackend"
Comment on lines -2335 to +2339
Copy link
Member

Choose a reason for hiding this comment

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

Will this cause data loss in self-hosted? I don't remember if self-hosted ever had release health but if they did, they will lose access to that data now.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm not sure we ever put a proper migration in place for self hosted. We should make sure that self hosted keeps using the old backend until we do that.

Copy link
Member

Choose a reason for hiding this comment

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

No this won't, self-hosted has already cut over to use metrics release health.
https://github.com/getsentry/self-hosted/blob/b3d3ce06da1661eca62a5d1fd5112810d6bbd117/sentry/sentry.conf.example.py#L195

Copy link
Member

Choose a reason for hiding this comment

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

Did we actually backfill release health from sessions over to metrics? lgtm if so

Copy link
Member

Choose a reason for hiding this comment

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

We implemented a period where we dual wrote to sessions/metrics datasets. Then, after 3 months(~90 days), we cut over to reading from the metrics dataset

Copy link
Member

@wedamija wedamija Mar 20, 2024 •
edited

Choose a reason for hiding this comment

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

How does that work in self hosted? What if they come from a version that had sessions, and come straight to latest release using metric. How do you enforce a 90 day dual write there?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately we weren't able to provide a way to smoothly accommodate that scenario. We gave self-hosted users as many warnings as we could through documentation and release notes months in advance before cutting over the metrics.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I don't think this is a good way to handle situations like this going forward. We could have provided a dual write backend that logs a date, dual writes, and starts reading from sessions going forward. Another option could have been to write a backfill that is only applied in self hosted. We really shouldn't be making self hosted a second class citizen like this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree with you there. There are definitely ways that this could've gone better but it also feels like a part of the broader issue that teams are shipping features without keeping self-hosted in mind.

cc @chadwhitacre

SENTRY_RELEASE_MONITOR_OPTIONS: dict[str, Any] = {}

# Render charts on the backend. This uses the Chartcuterie external service.
Expand Down
2 changes: 1 addition & 1 deletion tests/snuba/rules/conditions/test_event_frequency.py
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@
from sentry.testutils.skips import requires_snuba
from sentry.utils.samples import load_data

pytestmark = [requires_snuba]
pytestmark = [pytest.mark.sentry_metrics, requires_snuba]
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

any attempt to access metrics in tests seems to fail without it



class ErrorEventMixin(SnubaTestCase):
Expand Down