Skip to content

ref(sessions) Change sessions interval calculation to align with metric intervals#60466

Merged
RaduW merged 11 commits into
masterfrom
ref/metrics/align-sessions-api-intervals
Nov 29, 2023
Merged

ref(sessions) Change sessions interval calculation to align with metric intervals#60466
RaduW merged 11 commits into
masterfrom
ref/metrics/align-sessions-api-intervals

Conversation

@RaduW

@RaduW RaduW commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

This PR changes the interval calculation for the sessions api to align to the interval calculation of metrics api.

In short, given a start_time, end_time and an interval the calculated intervals are going to:

  • start at start_time if start_time is aligned with interval or at the first time before start_time that is aligned with interval.
  • end at end_time if end_time is aligned with interval or at the first time after end_time that is aligned with interval.

resolves #59686

change interval calculation
fix tests (with the exception of test_interval_restrictions)
@RaduW RaduW requested review from a team and Swatinem November 22, 2023 16:11
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 22, 2023
@codecov

codecov Bot commented Nov 23, 2023

Copy link
Copy Markdown

Codecov Report

Merging #60466 (f57bb19) into master (10beb79) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head f57bb19 differs from pull request most recent head 85de224. Consider uploading reports for the commit 85de224 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #60466      +/-   ##
==========================================
- Coverage   80.89%   80.89%   -0.01%     
==========================================
  Files        5185     5185              
  Lines      227508   227491      -17     
  Branches    38244    38237       -7     
==========================================
- Hits       184035   184020      -15     
  Misses      37854    37854              
+ Partials     5619     5617       -2     
Files Coverage Δ
src/sentry/snuba/sessions_v2.py 94.25% <100.00%> (-0.29%) ⬇️

... and 16 files with indirect coverage changes

@Swatinem Swatinem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

assert response.status_code == 200, response.content
assert response.data == {
"start": "2021-03-14T12:00:00Z",
"end": "2021-03-14T12:28:00Z",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

as mentioned in chat, the previous hack / NOTE related to snuba caching was there to have fresher data.

In this example, the end extends past the "current" (frozen) time. Because of the caching based on the end timestamp, you would only get fresh data once every 5 minutes (the cache time back when I implemented that).
With the "cache busting", effectively truncating the end to the current time, you would get a more uptodate view of the most recent "now" interval.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying

@RaduW RaduW marked this pull request as ready for review November 27, 2023 16:33
@RaduW RaduW requested a review from a team November 27, 2023 16:33
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 14, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Different time buckets in metric vs session api

3 participants