From 5c841cbc65ace4c054c23d426b14f2d7412f5d36 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Piotr=20Ma=C5=9Blanka?= <piotr.maslanka@henrietta.com.pl>
Date: Wed, 25 Aug 2021 23:26:12 +0200
Subject: [PATCH] add metrics and traces
---
README.md | 37 +++++++++++++-
docker-compose.yml | 3 +-
netguru/settings.py | 43 ++++++++++++----
requirements.txt | 2 +
shares/cron.py | 19 +++++--
shares/templates/share/view.html | 1 +
shares/tests.py | 16 ++++--
shares/views.py | 86 +++++++++++++++++++-------------
8 files changed, 151 insertions(+), 56 deletions(-)
diff --git a/README.md b/README.md
index 0a381e6..98e37f9 100644
--- a/README.md
+++ b/README.md
@@ -25,7 +25,11 @@ docker-compose up -d run_local
Deployment is handled automatically via a
[CI script](.gitlab-ci.yml).
-Deployment is done to a Docker Swarm platform.
+Deployment is done to a Docker Swarm platform,
+since I'm a big fan, and I'm still learning Kubernetes.
+
+I realize that Docker Swarm is a dead end, as it isn't supported
+anymore, but I've got 1,5-FTE worth of jobs on my hands.
### Configuration
@@ -48,3 +52,34 @@ but since it has already been used it isn't safe anymore.
The application needs a volume marked at
`/data` to store files. Because storing
files in a database is an antipattern.
+
+### Monitoring
+
+#### Logs
+
+You will just have to forgive me that I failed
+to hook up any loggers for it. Any Python-based
+logger will do, I sincerely recommends
+[seq-log](https://github.com/tintoy/seqlog)
+- full disclosure: I'm a contributor there
+
+#### Metrics
+
+The solution actively exports metrics at port 81.
+You can hook up Prometheus to it.
+It exports the metrics thanks to my excellent
+[satella](https://github.com/piotrmaslanka/satella)
+library and [django-satella-metrics](https://github.com/piotrmaslanka/django-satella-metrics)
+adapter.
+
+#### Traces
+
+The solution fully supports tracing. You can tweak
+the settings in [settings.py](netguru/settings.py) file
+to install for example a Jaeger instance.
+
+I failed to deploy that on your test environment since I'm
+pretty much strapped for time.
+
+BTW: If you're not already doing
+[tracing](https://opentracing.io/), you should totally consider it.
diff --git a/docker-compose.yml b/docker-compose.yml
index 269d93a..cc0ad77 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -12,7 +12,7 @@ services:
command: "./test.sh"
build: .
environment:
- - CI=1
+ - DEBUG-1
depends_on:
- postgres
run_local:
@@ -21,3 +21,4 @@ services:
- postgres
ports:
- 80:80
+ - 81:81
diff --git a/netguru/settings.py b/netguru/settings.py
index 8cd17bc..5f7a836 100644
--- a/netguru/settings.py
+++ b/netguru/settings.py
@@ -1,6 +1,7 @@
import os
+from satella.instrumentation.metrics import getMetric
+from satella.instrumentation.metrics.exporters import PrometheusHTTPExporterThread
-# Build paths inside the project like this: os.path.join(BASE_DIR, ...)
BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
# Quick-start development settings - unsuitable for production
@@ -10,7 +11,7 @@ BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
SECRET_KEY = os.environ.get('SECRET_KEY', 'r6(!w-%glre916esjft0^lds4u)=fym=v*26$@*&y7qcssfi%j')
# SECURITY WARNING: don't run with debug turned on in production!
-DEBUG = True
+DEBUG = 'DEBUG' in os.environ
ALLOWED_HOSTS = ['*']
@@ -27,6 +28,8 @@ INSTALLED_APPS = [
]
MIDDLEWARE = [
+ 'django_satella_metrics.DjangoSatellaMetricsMiddleware',
+ 'django_opentracing.OpenTracingMiddleware',
'django.middleware.security.SecurityMiddleware',
'django.contrib.sessions.middleware.SessionMiddleware',
'django.middleware.common.CommonMiddleware',
@@ -60,22 +63,16 @@ WSGI_APPLICATION = 'netguru.wsgi.application'
# Database
# https://docs.djangoproject.com/en/3.0/ref/settings/#databases
-if True:
- db = {
+DATABASES = {
+ 'default': {
'ENGINE': 'django.db.backends.postgresql',
'NAME': os.environ.get('DB_NAME', 'postgres'),
'USER': os.environ.get('DB_USER', 'postgres'),
'PASSWORD': os.environ.get('DB_PASS', 'postgres'),
'HOST': os.environ.get('DB_HOST', 'postgres'),
'PORT': int(os.environ.get('DB_PORT', '5432')),
- 'ATOMIC_REQUESTS': True # transactions aren't just a good idea
+ 'ATOMIC_REQUESTS': True # thank God for transactions
}
-else:
- db = {'ENGINE': 'django.db.backends.sqlite3',
- 'NAME': 'test-db.sqlite'}
-
-DATABASES = {
- 'default': db
}
# Password validation
@@ -116,3 +113,27 @@ STATIC_URL = '/static/'
STATIC_ROOT = os.path.join(BASE_DIR, 'media')
FILE_SAVE_LOCATION = '/data'
+
+# Configure tracing
+OPENTRACING_TRACE_ALL = True
+
+# Callable that returns an `opentracing.Tracer` implementation.
+OPENTRACING_TRACER_CALLABLE = 'opentracing.Tracer'
+
+# Parameters for the callable (Depending on the tracer implementation chosen)
+OPENTRACING_TRACER_PARAMETERS = {
+}
+
+# Set up metrics
+DJANGO_SATELLA_METRICS = {
+ 'summary_metric': getMetric('netguru.time.summary', 'summary'),
+ 'histogram_metric': getMetric('netguru.time.histogram', 'histogram'),
+ 'status_codes_metric': getMetric('netguru.status_codes', 'counter')
+}
+
+
+
+phet = PrometheusHTTPExporterThread('0.0.0.0', 81, {'service_name': 'netguru'})
+phet.start()
+
+
diff --git a/requirements.txt b/requirements.txt
index 7f6d31d..1b1b3ef 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -3,3 +3,5 @@ psycopg2
gunicorn
django_cron
satella
+django-satella-metrics
+django_opentracing
diff --git a/shares/cron.py b/shares/cron.py
index 34e80cb..6018a96 100644
--- a/shares/cron.py
+++ b/shares/cron.py
@@ -2,15 +2,20 @@ import logging
import os
from datetime import datetime, timedelta
from django_cron import CronJobBase, Schedule
+from satella.instrumentation.metrics import getMetric
from netguru.settings import FILE_SAVE_LOCATION
from .models import Share, SHARE_FILE
logger = logging.getLogger(__name__)
+links_deleted = getMetric('deleted.links', 'counter')
+files_deleted = getMetric('deleted.files', 'counter')
class ClearExpiredLinks(CronJobBase):
- RUN_EVERY_MINS = 60 # every 2 hours
+ # the links might be removed late by 59 minutes, but that shouldn't be a big deal
+ # I could if the handler to check exactly for expiration, but I won't do that.
+ RUN_EVERY_MINS = 60 # every hour
schedule = Schedule(run_every_mins=RUN_EVERY_MINS)
code = 'clear-expired-links' # a unique code
@@ -21,7 +26,15 @@ class ClearExpiredLinks(CronJobBase):
if share.share_type == SHARE_FILE:
path = os.path.join(FILE_SAVE_LOCATION, share.resource.split('.', 1)[0])
if os.path.exists(path):
- # we might have deleted in earlier and this call might have aborted with exception
- # the transaction won't proceed so we might have already deleted this file
+ # we might have deleted in earlier and this call might have aborted with
+ # exception, the transaction won't proceed so we might have already deleted this
+ # file. I realize that EAFP would be more Pythonic, but who cares?
+ logger.info('File %s deleted', path)
os.unlink(path)
+ files_deleted.runtime(+1) # I'm typing +1 on purpose, since it will
+ # increment the counter
+ else:
+ links_deleted.runtime(+1)
+ logger.info('A link was deleted')
+ share.delete()
logger.info('Deleted expired shares')
diff --git a/shares/templates/share/view.html b/shares/templates/share/view.html
index b8b3b3f..8d2d8da 100644
--- a/shares/templates/share/view.html
+++ b/shares/templates/share/view.html
@@ -9,5 +9,6 @@
{{ form.as_p }}
<p>
<input type="submit" value="Submit">
+ </p>
</form>
{% endblock %}
diff --git a/shares/tests.py b/shares/tests.py
index 1ef9aa9..c526eec 100644
--- a/shares/tests.py
+++ b/shares/tests.py
@@ -1,4 +1,11 @@
-from django.contrib.auth.hashers import make_password
+#
+# APOLOGIES IN ADVANCE!!!
+#
+# I realize that I would be best practice to deduplicate some code contained here
+# but since I'm running about 1,5 of full-time equvialents you just have to forgive me
+# for sparing the effort to do so. Thanks "from the mountain!"
+#
+
from django.contrib.auth.models import User
from django.test import TestCase, Client
from django.test.html import parse_html
@@ -13,17 +20,16 @@ def find_element(document, field, value):
return document
for child in document.children:
a = find_element(child, field, value)
- if a is not None:
+ if a:
return a
-# Create your tests here.
class TestShares(TestCase):
def setUp(self) -> None:
super().setUp()
- user = User.objects.create(username='testuser', email='admin@admin.com',
- password='12345')
+ user, _ = User.objects.get_or_create(username='testuser', email='admin@admin.com',
+ password='12345')
self.client = Client() # May be you have missed this line
self.client.force_login(user)
diff --git a/shares/views.py b/shares/views.py
index 5e55355..a82e16f 100644
--- a/shares/views.py
+++ b/shares/views.py
@@ -2,22 +2,29 @@ import mimetypes
import typing as tp
import os
import random
+import logging
import string
-from email.header import Header
import uuid
from django import forms
from django.core.validators import URLValidator
from django.forms import fields
from django.core.exceptions import ValidationError
-from django.http import Http404, StreamingHttpResponse
+from django.http import Http404, StreamingHttpResponse, HttpResponse
from django.shortcuts import render, get_object_or_404, redirect
from django.contrib.auth.decorators import login_required
from django.utils.safestring import mark_safe
+from satella.instrumentation.metrics import getMetric
from shares.models import Share, SHARE_URL, SHARE_FILE
import hashlib
from netguru.settings import SECRET_KEY, FILE_SAVE_LOCATION
+files_created = getMetric('created.files', 'counter')
+links_created = getMetric('created.links', 'counter')
+files_visited = getMetric('visited.files', 'counter')
+links_visited = getMetric('visited.links', 'counter')
+logger = logging.getLogger(__name__)
+
def hash_password(password: str) -> str:
return hashlib.sha256(password.encode('utf-8') + SECRET_KEY.encode('utf-8')).hexdigest()
@@ -25,12 +32,17 @@ def hash_password(password: str) -> str:
def generate_correct_uuid() -> str:
f = uuid.uuid4().hex
- # I know that hash collisions practically don't happen, but since it's so cheap for us
+ # I know that UUID collisions practically don't happen, but since it's so cheap for us
# to go that extra mile
+ # and since we're at risk at overwriting somebody's files, I guess i should check it
+ # I realize that it's susceptible to race conditions, but then again what is the chance?
+ # The chance that UUID will repeat during the 24 hours a link is active is much higher
+ # than the chance that it will repeat during the same 2 second window
while os.path.exists(FILE_SAVE_LOCATION, f):
f = uuid.uuid4().hex
return f
+
# Cut off O, I and S so that the user doesn't confuse them with 0, 1 and 5 respectively
CHARACTERS_TO_USE = string.ascii_uppercase.replace('O', '') \
.replace('I', '').replace('S', '') + string.digits
@@ -69,8 +81,8 @@ def add_share(request):
resource=data['url'],
pwd_hash=pwd_hash,
share_type=SHARE_URL)
- share.save()
- data_added['url'] = mark_safe(f'https://{request.get_host()}/shares/{share.id}')
+ links_created.runtime(+1)
+ logger.info('Created a link')
else:
file_name = generate_correct_uuid()
file = request.FILES['file']
@@ -82,8 +94,9 @@ def add_share(request):
resource=resource_name,
pwd_hash=pwd_hash,
share_type=SHARE_FILE)
- share.save()
- data_added['url'] = mark_safe(f'https://{request.get_host()}/shares/{share.id}')
+ files_created.runtime(+1)
+ data_added['url'] = mark_safe(f'https://{request.get_host()}/shares/{share.id}')
+ share.save()
else:
form = AddShareURLForm()
@@ -100,34 +113,37 @@ def view_share(request, share_id: str):
form = PasswordForm(request.POST)
if form.is_valid():
pwd = form.cleaned_data['password']
- if hash_password(pwd) == share.pwd_hash:
- try:
- if share.share_type == SHARE_URL:
- return redirect(share.resource)
- else:
- file_uuid, file_name = share.resource.split('.', 1)
- file_loc = os.path.join(FILE_SAVE_LOCATION, file_uuid)
- if not os.path.exists(file_loc):
- raise Http404()
- else:
- def file_iterator(path: str) -> tp.Iterator[bytes]:
- with open(path, 'rb') as f:
- p = f.read(1024)
- while p:
- yield p
- p = f.read(1024)
-
- # Django will have the UTF-8 mumbo jumbo for us
- cd = f'attachment; filename="{file_name}"'
- mimetype = mimetypes.guess_type(file_name)[0] or 'application/octet-stream'
- return StreamingHttpResponse(file_iterator(file_loc),
- content_type=mimetype,
- headers={
- 'Content-Disposition': cd
- })
- finally:
- share.views_count += 1
- share.save()
+ if hash_password(pwd) != share.pwd_hash:
+ return HttpResponse(status=401)
+ try:
+ if share.share_type == SHARE_URL:
+ links_visited.runtime(+1)
+ return redirect(share.resource)
+ else:
+ file_uuid, file_name = share.resource.split('.', 1)
+ file_loc = os.path.join(FILE_SAVE_LOCATION, file_uuid)
+ if not os.path.exists(file_loc):
+ raise Http404()
+
+ def file_iterator(path: str) -> tp.Iterator[bytes]:
+ with open(path, 'rb') as f:
+ p = f.read(1024)
+ while p:
+ yield p
+ p = f.read(1024)
+
+ # Django will have the UTF-8 mumbo jumbo for us
+ cd = f'attachment; filename="{file_name}"'
+ mimetype = mimetypes.guess_type(file_name)[0] or 'application/octet-stream'
+ files_visited.runtime(+1)
+ return StreamingHttpResponse(file_iterator(file_loc),
+ content_type=mimetype,
+ headers={
+ 'Content-Disposition': cd
+ })
+ finally:
+ share.views_count += 1
+ share.save()
else:
form = PasswordForm()
--
GitLab