Skip to content

Conversation

@jjbayer
Copy link
Member

@jjbayer jjbayer commented Oct 12, 2021

Release health stats stored as metrics can be queried with a resolution of up to 10 seconds. With this PR

  • the frontend requests data from the /sessions endpoint with an interval of "10s" if the total requested period is < 10 minutes.
  • Depending on which release health implementation is active, the backend then interprets "10s" as either 10 seconds (for metrics implementation) or as one minute (sessions implementation).
  • The frontend then displays the 10s-resolution data and adds seconds to the time format of the x-Axis label (done in ref(charts): Add seconds to x-Axis when interval equals 10s [INGEST-360] #29350).

This can be viewed on the release details page:

image

@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2021

size-limit report

Path Base Size (011c34f) Current Size Change
src/sentry/static/sentry/dist/entrypoints/app.js 52.74 KB 52.74 KB +0.01% 🔺
src/sentry/static/sentry/dist/entrypoints/sentry.css 70.89 KB 70.89 KB 0%

…-granularity-10s

 Conflicts:
	static/app/views/alerts/incidentRules/triggers/chart/thresholdsChart.tsx
type Props = Omit<ChartProps, 'series'> & {
stacked?: boolean;
series: AreaChartSeries[];
stacked?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

The frontend changes were already approved in #29350.

return datetime.now(tz=pytz.utc)


def get_constrained_date_range(
Copy link
Member Author

Choose a reason for hiding this comment

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

@Swatinem I'm not sure which of the rounding rules in this function should also apply to metrics-based queries, especially the one about real-time queries:

# snuba <-> sentry has a 5 minute cache for *exact* queries, which these
# are because of the way we do our rounding. For that reason we round the end
# of "realtime" queries to one minute into the future to get a one-minute cache instead.
if end > now:
end = to_datetime(ONE_MINUTE * (math.floor(to_timestamp(now) / ONE_MINUTE) + 1))

If this is specific to the "sessions" dataset in snuba, it might be better if I created a separate implementation of this function for metrics-based queries.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for this was that raw_query is being cached, keyed on the parameters you pass it (one of those parameters is the start/end times). That’s why this statement is there, to force a change in cache key and thus fetch a fresh dataset.

TBH, I have no idea if that is relevant for metrics-based sessions, probably not? I think you know better than me ;-)

Copy link
Member

Choose a reason for hiding this comment

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

after asking around in slack, there's two caches: one in sentry, one in snuba

snuba cache is i suppose what arpad is talking about.. tbh I am not sure if it ever caches more than 1 minute. sentry cache is disabled by default

I think the code is fine though... I don't get the disjunction between rounding_interval and interval, but rounding by 1h is too much. In any case the end result should probably be that we round the timestamps down to next 10s bucket

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you both, I will create an additional PR to split the logic for sessions- and metrics-based queries into two separate functions.

@jjbayer jjbayer marked this pull request as ready for review October 20, 2021 06:50
@jjbayer jjbayer requested a review from a team as a code owner October 20, 2021 06:50
@jjbayer jjbayer requested a review from a team October 20, 2021 06:50
@jjbayer jjbayer requested a review from a team as a code owner October 20, 2021 06:50
@jjbayer jjbayer requested review from a team, priscilawebdev and untitaker October 20, 2021 06:50
return datetime.now(tz=pytz.utc)


def get_constrained_date_range(
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for this was that raw_query is being cached, keyed on the parameters you pass it (one of those parameters is the start/end times). That’s why this statement is there, to force a change in cache key and thus fetch a fresh dataset.

TBH, I have no idea if that is relevant for metrics-based sessions, probably not? I think you know better than me ;-)

return datetime.now(tz=pytz.utc)


def get_constrained_date_range(
Copy link
Member

Choose a reason for hiding this comment

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

after asking around in slack, there's two caches: one in sentry, one in snuba

snuba cache is i suppose what arpad is talking about.. tbh I am not sure if it ever caches more than 1 minute. sentry cache is disabled by default

I think the code is fine though... I don't get the disjunction between rounding_interval and interval, but rounding by 1h is too much. In any case the end result should probably be that we round the timestamps down to next 10s bucket

@jjbayer jjbayer merged commit 6ed50d7 into master Oct 22, 2021
@jjbayer jjbayer deleted the feat/release-health-granularity-10s branch October 22, 2021 07:01
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2021
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.

5 participants