sentry

sentry copied to clipboard

In several locations we use a parameter called statsPeriod in the API. Its original use was defined to adjust the period of time that statistics were calculated for the given results (specifically it would adjust the small bar graph used in the UI). Several years ago however it was changed in one endpoint to have a complete different meaning:

https://github.com/getsentry/sentry/pull/11464

This creates a few problems:

1. we have a shared API parameter with explicitly different behavior
2. the new meaning of the API parameter does not reflect the name of the parameter well (its actually a results limiter, not a 'stats period')
3. many of these endpoints are undocumented, so differing behavior becomes even worse

To correct this, we could take the following approach:

1. Deprecate all statsPeriod parameters - undocument them entirely
2. Add new parameters that are appropriate for different behaviors. One for the historical statsPeriod (or leave its singular? existing use which is correct), and one for "limit results to this period of time".
3. Alias statsPeriod in all endpoints to whatever this new parameter is. so it can still function and we can migrate UI code and documentation

Routing to @getsentry/ecosystem for triage. ⏲️

Unfortunately we use statsPeriod a LOT in UI code. There are ~400 instances of it right now. A majority of them are likely for the variant of this parameter that limits the results.

Thre is also groupStatsPeriod in relation to issues, which does what you mentioned (specifically it would adjust the small bar graph used in the UI)

One last note- the worst outcome of this is the main endpoint we document (which is also one of the most critical use cases for customers), is the one with the original use of statsPeriod. That also means its the most concerning to change, and is a key component in the self-documenting nature of the other endpoints.

I think if we can come up with an incremental approach to make the change to the other (mostly undocumented) endpoints (without breaking them), even if its a lot of actual param name changes, we'll be in a much better position down the road. Its worthwhile to make these kinds of changes because APIs last a long time and refactorings/improvements add a large amount of value.

So we do have a backlog item to add a common statsPeriod validator. Is this what you had in mind @dcramer (with the alias for the nonstandard statsPeriod)?

@AniketDas-Tekky validation isnt the issue - the parameter name and meaning are. There are two different uses for this parameter, and one is not accurately named for what its behavior is (the one we have made very universal).

"groupStatsPeriod" on org issues, and "statsPeriod" on project issues are the ones that are appropriately named (they are the original use of the param). The "statsPeriod" used elsewhere is literally an interval selector for the data returned, and has nothing to do with stats. IMO we should rename that, and just use an undocumented field alias for compatibility (so you can still safely pass statsPeriod and nothing breaks, but we fix all our uses and publicly document a new, more appropriately named parameter).

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀