darkhelm/notifications-api
1
0
Fork 0
mirror of https://github.com/GSA/notifications-api.git synced 2026-08-26 01:03:41 -04:00
Code Issues Packages Projects Releases Wiki Activity

Refactor statsd logging

Browse Source 
Removed all existing statsd logging and replaced with:

- statsd decorator. Infers the stat name from the decorated function call. Delegates statsd call to statsd client. Calls incr and timing for each decorated method. This is applied to all tasks and all dao methods that touch the notifications/notification_history tables

- statsd client changed to prefix all stats with "notification.api."

- Relies on https://github.com/alphagov/notifications-utils/pull/61 for request logging. Once integrated we pass the statsd client to the logger, allowing us to statsd all API calls. This passes in the start time and the method to be called (NOT the url) onto the global flask object. We then construct statsd counters and timers in the following way

	notifications.api.POST.notifications.send_notification.200

This should allow us to aggregate to the level of

	- API or ADMIN
	- POST or GET etc
	- modules
	- methods
	- status codes

Finally we count the callbacks received from 3rd parties to mapped status.
This commit is contained in:
Martyn Inglis
2016-08-05 10:44:43 +01:00
parent 3128e79e7c
commit f223446f73
18 changed files with 121 additions and 223 deletions
Download Patch File Download Diff File Expand all files Collapse all files

72
tests/app/celery/test_provider_tasks.py
View File

@@ -20,6 +20,11 @@ from app.models import Notification, NotificationStatistics, Job, KEY_TYPE_NORMA
from tests.app.conftest import sample_notification
def test_should_have_decorated_tasks_functions():
assert send_sms_to_provider.__wrapped__.__name__ == 'send_sms_to_provider'
assert send_email_to_provider.__wrapped__.__name__ == 'send_email_to_provider'
def test_should_by_10_second_delay_as_default():
assert provider_tasks.retry_iteration_to_delay() == 10
@@ -91,9 +96,6 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
mocker.patch('app.mmg_client.send_sms')
mocker.patch('app.mmg_client.get_name', return_value="mmg")
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing_with_dates')
mocker.patch('app.statsd_client.timing')
send_sms_to_provider(
db_notification.service_id,
@@ -130,9 +132,6 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist
mocker.patch('app.aws_ses_client.send_email', return_value='reference')
mocker.patch('app.aws_ses_client.get_name', return_value="ses")
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing_with_dates')
mocker.patch('app.statsd_client.timing')
send_email_to_provider(
db_notification.service_id,
@@ -297,29 +296,6 @@ def test_should_not_send_to_provider_when_status_is_not_created(notify_db, notif
app.celery.research_mode_tasks.send_sms_response.apply_async.assert_not_called()
def test_send_sms_statsd_updates(notify_db, notify_db_session, sample_service, sample_notification, mocker):
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing')
mocker.patch('app.mmg_client.send_sms')
mocker.patch('app.mmg_client.get_name', return_value="mmg")
send_sms_to_provider(
sample_notification.service_id,
sample_notification.id
)
statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms-to-provider")
statsd_client.timing.assert_has_calls([
call("notifications.tasks.send-sms-to-provider.task-time", ANY),
call("notifications.sms.total-time", ANY)
])
# assert that the ANYs above are at least floats
for call_arg in statsd_client.timing.call_args_list:
assert isinstance(call_arg[0][1], float)
def test_should_go_into_technical_error_if_exceeds_retries(
notify_db,
notify_db_session,
@@ -329,8 +305,6 @@ def test_should_go_into_technical_error_if_exceeds_retries(
notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
service=sample_service, status='created')
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing')
mocker.patch('app.mmg_client.send_sms', side_effect=SmsClientException("EXPECTED"))
mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry', side_effect=MaxRetriesExceededError())
@@ -340,8 +314,6 @@ def test_should_go_into_technical_error_if_exceeds_retries(
)
provider_tasks.send_sms_to_provider.retry.assert_called_with(queue='retry', countdown=10)
assert statsd_client.incr.assert_not_called
assert statsd_client.timing.assert_not_called
db_notification = Notification.query.filter_by(id=notification.id).one()
assert db_notification.status == 'technical-failure'
@@ -369,9 +341,6 @@ def test_should_send_sms_sender_from_service_if_present(
mocker.patch('app.mmg_client.send_sms')
mocker.patch('app.mmg_client.get_name', return_value="mmg")
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing_with_dates')
mocker.patch('app.statsd_client.timing')
send_sms_to_provider(
db_notification.service_id,
@@ -450,8 +419,6 @@ def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retrie
notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
service=sample_service, status='created', template=sample_email_template)
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing')
mocker.patch('app.aws_ses_client.send_email', side_effect=EmailClientException("EXPECTED"))
mocker.patch('app.celery.provider_tasks.send_email_to_provider.retry', side_effect=MaxRetriesExceededError())
@@ -461,8 +428,6 @@ def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retrie
)
provider_tasks.send_email_to_provider.retry.assert_called_with(queue='retry', countdown=10)
assert statsd_client.incr.assert_not_called
assert statsd_client.timing.assert_not_called
db_notification = Notification.query.filter_by(id=notification.id).one()
assert db_notification.status == 'technical-failure'
@@ -474,31 +439,6 @@ def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retrie
assert job.notifications_failed == 1
def test_send_email_to_provider_statsd_updates(notify_db, notify_db_session, sample_service,
sample_email_template, mocker):
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing')
mocker.patch('app.aws_ses_client.send_email', return_value='reference')
mocker.patch('app.aws_ses_client.get_name', return_value="ses")
notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session,
template=sample_email_template)
send_email_to_provider(
notification.service_id,
notification.id
)
statsd_client.incr.assert_called_once_with("notifications.tasks.send-email-to-provider")
statsd_client.timing.assert_has_calls([
call("notifications.tasks.send-email-to-provider.task-time", ANY),
call("notifications.email.total-time", ANY)
])
# assert that the ANYs above are at least floats
for call_arg in statsd_client.timing.call_args_list:
assert isinstance(call_arg[0][1], float)
def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_created(notify_db, notify_db_session,
sample_service,
sample_email_template,
@@ -525,8 +465,6 @@ def test_send_email_should_use_service_reply_to_email(
sample_service,
sample_email_template,
mocker):
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing')
mocker.patch('app.aws_ses_client.send_email', return_value='reference')
mocker.patch('app.aws_ses_client.get_name', return_value="ses")

8
tests/app/celery/test_scheduled_tasks.py
View File

@@ -11,6 +11,14 @@ from app.celery.scheduled_tasks import (delete_verify_codes,
from tests.app.conftest import sample_notification
def test_should_have_decorated_tasks_functions():
assert delete_verify_codes.__wrapped__.__name__ == 'delete_verify_codes'
assert delete_successful_notifications.__wrapped__.__name__ == 'delete_successful_notifications'
assert delete_failed_notifications.__wrapped__.__name__ == 'delete_failed_notifications'
assert timeout_notifications.__wrapped__.__name__ == 'timeout_notifications'
assert delete_invitations.__wrapped__.__name__ == 'delete_invitations'
def test_should_call_delete_notifications_more_than_week_in_task(notify_api, mocker):
mocked = mocker.patch('app.celery.scheduled_tasksgit .delete_notifications_created_more_than_a_week_ago')
delete_successful_notifications()

26
tests/app/celery/test_tasks.py
View File

@@ -7,10 +7,10 @@ from mock import ANY
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.orm.exc import NoResultFound
from app import (encryption, DATETIME_FORMAT, statsd_client)
from app import (encryption, DATETIME_FORMAT)
from app.celery import provider_tasks
from app.celery import tasks
from app.celery.tasks import s3
from app.celery.tasks import s3, remove_job
from app.celery.tasks import (
send_sms,
process_job,
@@ -53,10 +53,15 @@ def _notification_json(template, to, personalisation=None, job_id=None, row_numb
return notification
def test_should_have_decorated_tasks_functions():
assert process_job.__wrapped__.__name__ == 'process_job'
assert remove_job.__wrapped__.__name__ == 'remove_job'
assert send_sms.__wrapped__.__name__ == 'send_sms'
assert send_email.__wrapped__.__name__ == 'send_email'
@freeze_time("2016-01-01 11:09:00.061258")
def test_should_process_sms_job(sample_job, mocker, mock_celery_remove_job):
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing')
mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms'))
mocker.patch('app.celery.tasks.send_sms.apply_async')
mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
@@ -81,8 +86,6 @@ def test_should_process_sms_job(sample_job, mocker, mock_celery_remove_job):
)
job = jobs_dao.dao_get_job_by_id(sample_job.id)
assert job.status == 'finished'
statsd_client.incr.assert_called_once_with("notifications.tasks.process-job")
statsd_client.timing.assert_called_once_with("notifications.tasks.process-job.task-time", ANY)
@freeze_time("2016-01-01 11:09:00.061258")
@@ -278,9 +281,6 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi
notification = _notification_json(sample_template_with_placeholders,
to="+447234123123", personalisation={"name": "Jo"})
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing_with_dates')
mocker.patch('app.statsd_client.timing')
mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async')
notification_id = uuid.uuid4()
@@ -292,15 +292,12 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi
datetime.utcnow().strftime(DATETIME_FORMAT)
)
statsd_client.timing.assert_called_once_with("notifications.tasks.send-sms.task-time", ANY)
provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with(
(sample_template_with_placeholders.service_id,
notification_id),
queue="sms"
)
statsd_client.incr.assert_called_once_with("notifications.tasks.send-sms")
persisted_notification = Notification.query.filter_by(id=notification_id).one()
assert persisted_notification.id == notification_id
assert persisted_notification.to == '+447234123123'
@@ -436,9 +433,6 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh
"my_email@my_email.com",
{"name": "Jo"},
row_number=1)
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing_with_dates')
mocker.patch('app.statsd_client.timing')
mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async')
notification_id = uuid.uuid4()
@@ -456,8 +450,6 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh
key_type=KEY_TYPE_TEAM
)
statsd_client.incr.assert_called_once_with("notifications.tasks.send-email")
statsd_client.timing.assert_called_once_with("notifications.tasks.send-email.task-time", ANY)
persisted_notification = Notification.query.filter_by(id=notification_id).one()
provider_tasks.send_email_to_provider.apply_async.assert_called_once_with(
(sample_email_template_with_placeholders.service_id, notification_id), queue='email')

6
tests/app/dao/test_services_dao.py
View File

@@ -44,6 +44,12 @@ from tests.app.conftest import (
)
def test_should_have_decorated_services_dao_functions():
assert dao_fetch_weekly_historical_stats_for_service.__wrapped__.__name__ == 'dao_fetch_weekly_historical_stats_for_service' # noqa
assert dao_fetch_todays_stats_for_service.__wrapped__.__name__ == 'dao_fetch_todays_stats_for_service' # noqa
assert dao_fetch_stats_for_service.__wrapped__.__name__ == 'dao_fetch_stats_for_service' # noqa
def test_create_service(sample_user):
assert Service.query.count() == 0
service = Service(name="service_name",

33
tests/app/notifications/rest/test_callbacks.py
View File

@@ -542,34 +542,6 @@ def test_ses_callback_should_update_multiple_notification_status_sent(
assert stats.emails_failed == 0
def test_ses_callback_should_update_record_statsd(
notify_api,
notify_db,
notify_db_session,
sample_email_template,
mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr')
notification = create_sample_notification(
notify_db,
notify_db_session,
template=sample_email_template,
reference='ref',
status='sending'
)
assert get_notification_by_id(notification.id).status == 'sending'
client.post(
path='/notifications/email/ses',
data=ses_notification_callback(),
headers=[('Content-Type', 'text/plain; charset=UTF-8')]
)
app.statsd_client.incr.assert_called_once_with("notifications.callback.ses.delivered")
def test_ses_callback_should_set_status_to_temporary_failure(notify_api,
notify_db,
notify_db_session,
@@ -707,9 +679,7 @@ def test_process_mmg_response_records_statsd(notify_api, sample_notification, mo
client.post(path='notifications/sms/mmg',
data=data,
headers=[('Content-Type', 'application/json')])
assert app.statsd_client.incr.call_count == 2
app.statsd_client.incr.assert_any_call("notifications.callback.mmg.delivered")
app.statsd_client.incr.assert_any_call("notifications.callback.mmg.status.3")
def test_firetext_callback_should_record_statsd(notify_api, notify_db, notify_db_session, mocker):
@@ -725,9 +695,6 @@ def test_firetext_callback_should_record_statsd(notify_api, notify_db, notify_db
),
headers=[('Content-Type', 'application/x-www-form-urlencoded')])
assert app.statsd_client.incr.call_count == 3
app.statsd_client.incr.assert_any_call("notifications.callback.firetext.code.101")
app.statsd_client.incr.assert_any_call("notifications.callback.firetext.status.0")
app.statsd_client.incr.assert_any_call("notifications.callback.firetext.delivered")

61
tests/app/notifications/rest/test_send_notification.py
View File

@@ -10,7 +10,7 @@ from notifications_python_client.authentication import create_jwt_token
import app
from app import encryption
from app.models import ApiKey, KEY_TYPE_TEAM, KEY_TYPE_TEST
from app.models import ApiKey, KEY_TYPE_TEAM
from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template
from app.dao.services_dao import dao_update_service
from app.dao.api_key_dao import save_model_api_key
@@ -37,7 +37,6 @@ def test_create_sms_should_reject_if_missing_required_fields(notify_api, sample_
headers=[('Content-Type', 'application/json'), auth_header])
json_resp = json.loads(response.get_data(as_text=True))
app.celery.tasks.send_sms.apply_async.assert_not_called()
assert json_resp['result'] == 'error'
assert 'Missing data for required field.' in json_resp['message']['to'][0]
assert 'Missing data for required field.' in json_resp['message']['template'][0]
@@ -313,13 +312,13 @@ def test_should_not_allow_template_from_another_service(notify_api, service_fact
]
)
def test_should_not_allow_template_content_too_large(
notify_api,
notify_db,
notify_db_session,
sample_user,
template_type,
mocker,
should_error
notify_api,
notify_db,
notify_db_session,
sample_user,
template_type,
mocker,
should_error
):
with notify_api.test_request_context():
with notify_api.test_client() as client:
@@ -655,50 +654,6 @@ def test_should_allow_api_call_if_under_day_limit_regardless_of_type(notify_db,
assert response.status_code == 201
def test_should_record_email_request_in_statsd(notify_api, sample_email_template, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr')
mocker.patch('app.celery.tasks.send_email.apply_async')
mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
data = {
'to': 'ok@ok.com',
'template': str(sample_email_template.id)
}
auth_header = create_authorization_header(service_id=sample_email_template.service_id)
response = client.post(
path='/notifications/email',
data=json.dumps(data),
headers=[('Content-Type', 'application/json'), auth_header])
assert response.status_code == 201
app.statsd_client.incr.assert_called_once_with("notifications.api.email")
def test_should_record_sms_request_in_statsd(notify_api, sample_template, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:
mocker.patch('app.statsd_client.incr')
mocker.patch('app.celery.tasks.send_sms.apply_async')
mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
data = {
'to': '07123123123',
'template': str(sample_template.id)
}
auth_header = create_authorization_header(service_id=sample_template.service_id)
response = client.post(
path='/notifications/sms',
data=json.dumps(data),
headers=[('Content-Type', 'application/json'), auth_header])
assert response.status_code == 201
app.statsd_client.incr.assert_called_once_with("notifications.api.sms")
def test_should_not_return_html_in_body(notify_api, notify_db, notify_db_session, mocker):
with notify_api.test_request_context():
with notify_api.test_client() as client:

24
tests/app/test_statsd_decorators.py Normal file
View File

@@ -0,0 +1,24 @@
from mock import ANY
from app.statsd_decorators import statsd
import app
class AnyStringWith(str):
def __eq__(self, other):
return self in other
def test_should_call_statsd(notify_api, mocker):
mocker.patch('app.statsd_client.incr')
mocker.patch('app.statsd_client.timing')
mock_logger = mocker.patch.object(notify_api.logger, 'info')
@statsd(namespace="test")
def test_function():
return True
assert test_function()
app.statsd_client.incr.assert_called_once_with("test.test_function")
app.statsd_client.timing.assert_called_once_with("test.test_function", ANY)
mock_logger.assert_called_once_with(AnyStringWith("test call test_function took "))