add automated tests to ensure that all queries have a /* query_name */ comment

VERIFIED FIXED

Status

Cloud Services
Server: Core
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: atoll, Assigned: rfkelly)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa+])

Attachments

(1 attachment)

Ops requires some sort of /* query_name */ to manage live databases in production, file intelligent bugs for dev, and track query performance to ops/dev/qa.  Existing queries will be updated to include names as part of 669804.

Please update the python test suites to warn developers when queries are created that do not have a name applied via with_hint().  This will ensure that all queries are named before they reach staging and production.
Assignee: nobody → rkelly
Primarily relevant for sync at this point.
(Assignee)

Comment 2

6 years ago
From Bug 669804 it sounds like using with_hint is no longer an option, so I've adjusted the bug title accordingly.
Summary: add automated tests to ensure that all queries are named via with_hint → add automated tests to ensure that all queries have a /* query_name */ comment
Whiteboard: [qa-]
(Assignee)

Comment 3

6 years ago
Adding an additional request from Bug 759038, since it will need to be done as part of this refactoring: queries that are being retried due to a connection error should have /* retried */ prepended as well so we can trace them in the logs.
Whiteboard: [qa-] → [qa+]
(Assignee)

Comment 4

6 years ago
Created attachment 644850 [details] [diff] [review]
query-name-annotation patch for sync2.0

Attaching a patch to implement this for sync2.0.

Since we have two pieces of information that we potentially want to capture ("queryName" and "retries") I have generalized this to a dict of "annotations" that can be specified for each query.  The queryName and retry status are filled in automatically, but calling code could override their values or provide additional fields if we need.

There is also a test that listens for all queries being sent through to the database, and asserts that they contain the necessary annotations.

:atoll, is it important for me to backport this to sync1.1 as well?  It will be a bit hackier but should be possible in a similar manner to the code shown here.
Attachment #644850 - Flags: review?(telliott)
Attachment #644850 - Flags: review?(telliott) → review+
(Assignee)

Comment 5

6 years ago
https://github.com/mozilla-services/server-syncstorage/commit/c1f36a801c1c5b2f36a80ad8d0a858978373f96b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

6 years ago
Erm, I should not have RESOLED/FIXED this because it hasn't been put into sync1.1 yet
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

4 years ago
This is in sync1.5, and we're not taking changes for sync1.1, so closing it out
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago4 years ago
Resolution: --- → FIXED
OK. Sounds good.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.