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.

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(replays): add public API endpoint documentation for replays index and details #53526

Merged
merged 16 commits into from Jul 26, 2023

Conversation

michellewzhang
Copy link
Member

@michellewzhang michellewzhang commented Jul 25, 2023

Added documentation for the replays index & replays details public endpoint API.
Relates to getsentry/team-replay#112

SCR-20230725-jlpo

Index:
SCR-20230725-jlwb
SCR-20230725-jlyk

Details:
SCR-20230725-jzky
SCR-20230725-jznq

@michellewzhang michellewzhang requested review from a team as code owners July 25, 2023 17:35
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 25, 2023

"""Return only the fields requested by the client."""
if fields:
for item in response:
yield {field: item[field] for field in fields}
yield {field: item[field] for field in fields} # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

there may be a way to fix so we don't have to type ignore, but reading here python/mypy#7178 seems like guaranteeing dictionary key access of TypeDicts are annoying. @getsentry/python-typing anyone here have ideas?

Copy link
Member

Choose a reason for hiding this comment

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

the fields should be a list[Literal['possible', 'keys', 'here']] | None I believe

though it feels weird to only return some of the fields like this -- are there explicit subsets that we can type directly rather than allowing any combination of fields to be sliced out?

Copy link
Member

Choose a reason for hiding this comment

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

unfortunately, no. The fields returned are specified by the user via query string to the endpoint.

Copy link
Member

Choose a reason for hiding this comment

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

but we can try the literal and see if that works.

Copy link
Member

Choose a reason for hiding this comment

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

can we just give them everything and remove / deprecate that behaviour? what's the risk in giving everything?

Copy link
Member

Choose a reason for hiding this comment

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

unfortunately some fields result in expensive clickhouse queries, and it's better to leave them off by default.

Copy link
Member

Choose a reason for hiding this comment

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

maybe I'm misreading this -- but haven't they already been fetched at this point?

Copy link
Member

Choose a reason for hiding this comment

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

right, yeah. we set default values below if none were returned from the query. we may be able to refactor to make a bit more type friendly/clear. thanks

@JoshFerge JoshFerge requested review from a team July 25, 2023 17:51
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #53526 (e0bc7eb) into master (7b1bd66) will increase coverage by 0.00%.
Report is 7 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #53526   +/-   ##
=======================================
  Coverage   79.55%   79.56%           
=======================================
  Files        4942     4943    +1     
  Lines      208999   209085   +86     
  Branches    35582    35590    +8     
=======================================
+ Hits       166270   166356   +86     
- Misses      37652    37654    +2     
+ Partials     5077     5075    -2     
Files Changed Coverage Δ
src/sentry/apidocs/build.py 25.00% <ø> (ø)
src/sentry/apidocs/examples/replay_examples.py 100.00% <100.00%> (ø)
...y/replays/endpoints/organization_replay_details.py 94.59% <100.00%> (+1.73%) ⬆️
...try/replays/endpoints/organization_replay_index.py 100.00% <100.00%> (ø)
src/sentry/replays/post_process.py 95.27% <100.00%> (+4.65%) ⬆️

... and 7 files with indirect coverage changes

@michellewzhang

This comment was marked as resolved.

@asottile-sentry

This comment was marked as resolved.

@michellewzhang michellewzhang changed the title feat(replays): add public API endpoint documentation for replays index feat(replays): add public API endpoint documentation for replays index and details Jul 25, 2023
def get(self, request: Request, organization: Organization, replay_id: str) -> Response:
"""
Retrieve a single replay instance.
Copy link
Member

Choose a reason for hiding this comment

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

More in line with what we have on our docs.

Suggested change
Retrieve a single replay instance.
Return details on an individual replay.

@@ -32,7 +42,21 @@ def get_replay_filter_params(self, request, organization):

return filter_params

@extend_schema(
operation_id="Query for a List of Replays",
Copy link
Member

Choose a reason for hiding this comment

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

Endpoints that return a list are usually titled List ... (eg: List an Organization's Teams). Ideally we would do the same here, but I'm unfamiliar with how replays work. Is the returned list of replays tied to some project or something else that we can include in the title?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change it to something like "List an Organization's Replays" since replays are associated by org slug

def get(self, request: Request, organization: Organization) -> Response:
"""
Query for a list of replays using the Replays GET Request.
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, ideally this would be like the description in List a Team's Projects

Suggested change
Query for a list of replays using the Replays GET Request.
Return a list of replays ...

@michellewzhang michellewzhang merged commit 10f9c5e into master Jul 26, 2023
56 of 57 checks passed
@michellewzhang michellewzhang deleted the mz/add-replay-idx-apidoc branch July 26, 2023 20:48
@michellewzhang michellewzhang restored the mz/add-replay-idx-apidoc branch July 26, 2023 23:38
@michellewzhang michellewzhang deleted the mz/add-replay-idx-apidoc branch July 26, 2023 23:38
@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 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.

None yet

4 participants