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(release-health): Sessions should not be default #67353
Changes from 13 commits
804e0e4
bea49a7
ca558be
482595b
ef22bc4
b88e787
d6184ea
c86e019
5dabab4
1ef5c0d
16142cf
62855c0
55401ea
929c1fa
7315406
42ac0cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,7 +66,7 @@ def new_build_results(*args, **kwargs): | |
is_performance_metrics = False | ||
is_metrics = False | ||
if not isinstance(query, MetricsQuery) and not isinstance(query.match, Join): | ||
is_performance_metrics = query.match.name.startswith("generic_metrics") | ||
is_performance_metrics = query.match.name.startswith("generic") | ||
is_metrics = "metrics" in query.match.name | ||
|
||
if is_performance_metrics: | ||
|
@@ -85,7 +85,7 @@ def new_create_snql_in_snuba(subscription, snuba_query, snql_query, entity_subsc | |
is_performance_metrics = False | ||
is_metrics = False | ||
if isinstance(query.match, Entity): | ||
is_performance_metrics = query.match.name.startswith("generic_metrics") | ||
is_performance_metrics = query.match.name.startswith("generic") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not the best fix but it was the easiest for the scope of this PR. We might want in the future to improve this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of curiosity, how does this change fix the tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The acceptance tests were running into some endpoints which were querying generic metrics but via the entity key |
||
is_metrics = "metrics" in query.match.name | ||
|
||
if is_performance_metrics: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,6 +86,7 @@ def adjust_end(end: datetime.datetime, interval: int) -> datetime.datetime: | |
return end | ||
|
||
|
||
@pytest.mark.xfail(reason="Does not work with the metrics release health backend") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iambriccardo any thoughts on this one? |
||
class OrganizationSessionsEndpointTest(APITestCase, SnubaTestCase): | ||
def setUp(self): | ||
super().setUp() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this do? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
@@ -281,6 +281,7 @@ def increment(self, event, count, environment=None, timestamp=None): | |
) | ||
|
||
|
||
@pytest.mark.xfail(reason="Does not work with the metrics release health backend") | ||
class EventFrequencyPercentConditionTestCase(SnubaTestCase, RuleTestCase): | ||
__test__ = Abstract(__module__, __qualname__) | ||
|
||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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