Label more solr queries for monitoring + some SubjectEngine, QueryLabel refactoring#11312
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds monitoring labels to subject page Solr queries and refactors the SubjectEngine class hierarchy. The main goal is to improve observability of subject and publisher page traffic in Solr dashboards by labeling queries with SUBJECT_ENGINE_PAGE and SUBJECT_ENGINE_API.
- Simplifies SubjectEngine class hierarchy by removing SubjectMeta and making SubjectEngine a dataclass
- Renames QueryLabel to SolrRequestLabel for broader future use with other Solr operations
- Adds request_label parameters throughout the codebase to properly label Solr queries
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| openlibrary/plugins/worksearch/subjects.py | Refactors SubjectEngine to dataclass, removes SubjectMeta, adds request labels |
| openlibrary/plugins/worksearch/publishers.py | Converts PublisherEngine to dataclass with proper inheritance |
| openlibrary/plugins/worksearch/languages.py | Converts LanguageEngine to dataclass with proper inheritance |
| openlibrary/plugins/worksearch/code.py | Renames QueryLabel to SolrRequestLabel and updates parameter names |
| openlibrary/plugins/openlibrary/partials.py | Updates parameter name from query_label to request_label |
| openlibrary/macros/RawQueryCarousel.html | Updates parameter name from query_label to request_label |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| engine = next((e for e in SUBJECTS if key.startswith(e.prefix)), None) | ||
|
|
||
| if not engine: | ||
| raise NotImplementedError(f"No SubjectEngine for key: {key}") |
There was a problem hiding this comment.
The error message uses NotImplementedError which typically indicates missing functionality, but this is actually a case of an invalid/unsupported key. Consider using ValueError with a more descriptive message like 'Unsupported subject key format: {key}'
| raise NotImplementedError(f"No SubjectEngine for key: {key}") | |
| raise ValueError(f"Unsupported subject key format: {key}") |
cdrini
added
the
Patch Deployed
|
Patch deploying to help get some visibility into the current influx of traffic |
cdrini
changed the title
cdrini
added
the
Priority: 1
We were today hit with high traffic to our subject and author pages, but were unable to see the labelled requests in our solr dashboards. The pages are now labelled with:
SUBJECT_ENGINE_PAGEandSUBJECT_ENGINE_API. Note this also includes/publisherpages, which are in essence the same.AUTHOR_BOOKS_PAGEfor author pagesGET_WORK_SOLR_DATAhas been added to label when solr data is fetched via theWork._solr_data()model method. This happens on a lot of pages, but is fast.UNLABELLEDhas also been modified to suffix with the type of request made, egUNABELLED_SELECT,UNLABELLED_GET, etc.Technical
SubjectEngineto have a simpler class hierarchy by removing theSubjectMetaclass. Now there is only oneSubjectEngineclass that is sub-classed for publishers and languages.QueryLabeltoSolrRequestLabelto allow us in the future to use it to also monitor e.g. solr/updatecalls.Testing
✅ ol-solr1 shows the new label after deploying to testing:
Screenshot
Stakeholders