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: add timing for results filtering #142

Merged
merged 11 commits into from
Jan 4, 2024

Conversation

davidnuon
Copy link
Member

@davidnuon davidnuon commented Dec 19, 2023

For KBK-57

Adds a edx.course.search.executed event with timings for

  • Filter generation
  • Total search time
  • Post-processing results

@davidnuon davidnuon marked this pull request as ready for review December 20, 2023 21:07
Copy link

@schenedx schenedx left a comment

Choose a reason for hiding this comment

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

See my comments inline.

search/api.py Outdated
processing_time['end'] = time.time()
search_time['end'] = time.time()

processing_time_in_seconds = processing_time['end'] - processing_time['start']

Choose a reason for hiding this comment

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

Can we please wrap all these into a separate function? That way, we can add a set of unit test to ensure the calculation and event sending

search/api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ashultz0 ashultz0 left a comment

Choose a reason for hiding this comment

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

looks good, a couple of extra fields would I think help in the end

also needs a bump in the version number which is in the weird little edxsearch application, maybe 3.8.0 for a nonbreaking but interesting change

we probably don't want to actually cut and release 3.8.0 until after the break but we can set up for it

@ashultz0
Copy link
Contributor

oh also you can probably collapse the commits together on/before merge, they don't look super informational

@davidnuon davidnuon force-pushed the feature/search-performance-logging branch 2 times, most recently from 621be29 to 9150b0a Compare January 4, 2024 20:37
@davidnuon davidnuon force-pushed the feature/search-performance-logging branch from 9150b0a to d2e29e0 Compare January 4, 2024 20:43
Copy link

@schenedx schenedx left a comment

Choose a reason for hiding this comment

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

👍

@davidnuon davidnuon merged commit 091a479 into master Jan 4, 2024
8 checks passed
@davidnuon davidnuon deleted the feature/search-performance-logging branch January 4, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants