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

Status: VERIFIED FIXED

(Cloud Services :: Server: Core, defect)

Description

[:Atoll]

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.

Comment 1

[Toby Elliott [:telliott]]

Primarily relevant for sync at this point.

Comment 2

[Ryan Kelly [:rfkelly]]

From Bug 669804 it sounds like using with_hint is no longer an option, so I've adjusted the bug title accordingly.

Comment 3

[Ryan Kelly [:rfkelly]]

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.

Comment 4

[Ryan Kelly [:rfkelly]]

Attachment: 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.

Comment 5

[Ryan Kelly [:rfkelly]]

https://github.com/mozilla-services/server-syncstorage/commit/c1f36a801c1c5b2f36a80ad8d0a858978373f96b

Comment 6

[Ryan Kelly [:rfkelly]]

Erm, I should not have RESOLED/FIXED this because it hasn't been put into sync1.1 yet

Comment 7

[Ryan Kelly [:rfkelly]]

This is in sync1.5, and we're not taking changes for sync1.1, so closing it out

Comment 8

[James Bonacci [:jbonacci]]

OK. Sounds good.