Added feature: exposing prometheus metrics by ahpooch · Pull Request #153 · Py-KMS-Organization/py-kms

ahpooch · 2026-07-03T23:29:36Z

This PR provides a solution to #152
It is a tested and working as expected code though written with AI support.
Based on the changes provided in this PR, I was able to build custom Docker image that exposes Prometheus metrics as proposed. After that I managed to create Grafana Dashboard for monitoring py-kms container metrics, and it looks solid (after 26 iterations of building image while working on this solution using AI and my humble programming skills).

The last image build from PR code is available here if somebody wants to test it:
https://hub.docker.com/repository/docker/ahpooch/py-kms/tags/prometheus-metrics26/sha256-f0b8449bdab21fb6491b5e5f3d0b209d96f9e09ba1b7bcb9004476b64193e259

Or you could build the Docker image yourself from the code of this PR after reading the changes it proproses to project.

Copilot

Pull request overview

Adds Prometheus-format metrics export to the py-kms python3 Docker image by integrating prometheus_client into the existing Flask WebUI and recording activation/database statistics.

Changes:

Add /metrics endpoint to the Flask WebUI and initialize service-level metrics.
Introduce a new pykms_Metrics module defining Prometheus counters/gauges/histograms and DB-derived metrics.
Wire activation request recording into the KMS request handling flow; add Gunicorn multiprocess support config and Docker startup/requirements updates.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
py-kms/pykms_WebUI.py	Adds Prometheus initialization and the `/metrics` HTTP endpoint in the WebUI Flask app.
py-kms/pykms_Metrics.py	New module implementing metric definitions, collection output, and DB/windowed helpers.
py-kms/pykms_Base.py	Records per-activation metrics from the KMS request handling logic.
py-kms/gunicorn_config.py	Adds Gunicorn hooks for Prometheus multiprocess lifecycle handling/cleanup.
docker/start.py	Sets `PROMETHEUS_MULTIPROC_DIR` and starts Gunicorn using the new config.
docker/docker-py3-kms/requirements.txt	Adds `prometheus-client` dependency for the python3 image.
docs/Metrics.md	Documents configuration and available Prometheus metrics.
examples/grafana-dashboard.json	Provides an example Grafana dashboard for the exported metrics.

Suppressed comments (4)

py-kms/pykms_Base.py:297

Same issue in the exception path: reading _metrics_* from the shared srv_config can race with other threads. Use getattr(messagehandler, ...) so failures are attributed to the correct request.

		app_name = srv_config.get('_metrics_app_name', 'unknown')
		sku_name = srv_config.get('_metrics_sku_name', 'unknown')

py-kms/pykms_Base.py:282

These values are read back from srv_config, which is shared across threads; this can pick up another request's _metrics_* values. If you store the names on messagehandler (per-request instance), read them via getattr(messagehandler, ...) here to avoid cross-thread contamination.

		# Get product and SKU from parsed data (set by serverLogic)
		app_name = srv_config.get('_metrics_app_name', 'unknown')
		sku_name = srv_config.get('_metrics_sku_name', 'unknown')

py-kms/pykms_Metrics.py:360

This INFO log runs for every activation request, which can flood logs under normal traffic. It should be DEBUG-level (or removed) to avoid operational noise and I/O overhead.

    loggersrv.info(f"Recording activation metric: status={status}, product={product}, kms_version={kms_version}, sku={sku}, duration={duration}")

py-kms/pykms_Metrics.py:395

This INFO log runs after every activation request and will be very noisy in practice. Prefer DEBUG-level logging here to keep normal logs usable.

        loggersrv.info(f"Successfully recorded activation metric")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

simonmicro · 2026-08-07T13:44:08Z

+                # Store app and SKU names for metrics (temporary storage)
+                self.srv_config['_metrics_app_name'] = appName
+                self.srv_config['_metrics_sku_name'] = skuName


well, as long Python had not gotten its global locking problem fixed, I do not see this one as bad - but should be redone anyways as it does feel a bit wrong

simonmicro · 2026-08-07T13:42:44Z

 from pykms_Filetimes import filetime_to_dt
 from pykms_Sql import sql_update, sql_update_epid
 from pykms_Format import justify, byterize, enco, deco, pretty_printer
+from pykms_Metrics import record_activation_request, get_product_type, get_kms_version, get_sku_label


💯 - damn, I was careful with the other imports and just overlooked this one as well

simonmicro · 2026-08-07T13:41:36Z

 def generateKmsResponseData(data, srv_config):
-        version = kmsBase.GenericRequestHeader(data)['versionMajor']
-        currentDate = time.strftime("%a %b %d %H:%M:%S %Y")
+	version = kmsBase.GenericRequestHeader(data)['versionMajor']
+	currentDate = time.strftime("%a %b %d %H:%M:%S %Y")

-        if version == 4:
-                loggersrv.info("Received V%d request on %s." % (version, currentDate))
-                messagehandler = pykms_RequestV4.kmsRequestV4(data, srv_config)     
-        elif version == 5:
-                loggersrv.info("Received V%d request on %s." % (version, currentDate))
-                messagehandler = pykms_RequestV5.kmsRequestV5(data, srv_config)
-        elif version == 6:
-                loggersrv.info("Received V%d request on %s." % (version, currentDate))
-                messagehandler = pykms_RequestV6.kmsRequestV6(data, srv_config)
-        else:
-                loggersrv.info("Unhandled KMS version V%d." % version)
-                messagehandler = pykms_RequestUnknown.kmsRequestUnknown(data, srv_config)
-
-        return messagehandler.executeRequestLogic()
+	if version == 4:
+		loggersrv.info("Received V%d request on %s." % (version, currentDate))
+		messagehandler = pykms_RequestV4.kmsRequestV4(data, srv_config)	


+            try:
+                clients = sql_get_all(dbPath)
+                pykms_Metrics.update_database_metrics(dbPath, clients)
+            except Exception as e:
+                pass  # Ignore errors when updating database metrics


simonmicro · 2026-08-07T13:44:39Z

+    except Exception as e:
+        return f'Error generating metrics: {e}', 500


💯 Hehe, exactly my point.

simonmicro · 2026-08-07T13:48:05Z

+        # Add event to shared shelve storage (for time-based metrics)
+        try:
+            with shelve.open(_EVENTS_DB_PATH) as db:
+                events = db.get('events', [])
+                events.append((time.time(), status, product))


Hmmm...

LLM#1: Shared storage for activation events using shelve (cross-process safe)
LLM#2: shelve/dbm files are not safe for concurrent writes across threads/processes

Fight!

No, for real this should be addressed. I have not used shelve before on my own, but I also suspected as such and got silenced by the comment I read. There are some mechanisms in Linux to grant exclusive file-access (which is the default under Microslop Windows), but one would need to explicitly enable those here first - which does also not feel right due to OS-specific calls...

+                    count_300s = counts_300s.get(key, 0)
+
+                    if count_30s > 0 or count_60s > 0 or count_300s > 0:
+                        loggersrv.info(f"Setting metrics for {status}/{product}: 30s={count_30s}, 60s={count_60s}, 300s={count_300s}")


simonmicro

ooofff... this is A LOT of code... after reading through it once I see much information duplication and repetition, which also blows this MR up and will impact future maintainability hard

please do not take my lazy comments personal, at some point the motivation just got a bit low as most of the questionable decisions were not taken by you, but some LLM - so I also likely overlooked a lot of stuff

I would welcome a rework of this with more attention to detail and reasoning of changes before they are just done 😉

thank you in advance

🐱

simonmicro · 2026-08-07T12:53:05Z

+    os.makedirs(prometheus_multiproc_dir, exist_ok=True)
+  # Set environment variables for this process and all child processes
+  os.environ['PROMETHEUS_MULTIPROC_DIR'] = prometheus_multiproc_dir
+  os.environ['prometheus_multiproc_dir'] = prometheus_multiproc_dir


In accordance to https://prometheus.github.io/client_python/multiprocess/ the variable name is uppercase only. No need for lowercase in addition.

simonmicro · 2026-08-07T12:54:47Z

 def start_kms(logger):
+  # Set up Prometheus multiprocess directory for both KMS Server and WebUI
+  # This must be done BEFORE starting any process that writes metrics
+  prometheus_multiproc_dir = '/tmp/prometheus_multiproc'


I do not like hard-coded temporary paths - especially ones, which refer to /tmp from the rootfs. Could you please revisit this one and utilize tempfile? This is handed down using the env-vars anyways already.

simonmicro · 2026-08-07T12:55:37Z

+      # Set up Prometheus multiprocess mode (for Gunicorn workers)
+      prometheus_multiproc_dir = '/tmp/prometheus_multiproc'
+      if not os.path.exists(prometheus_multiproc_dir):
+        os.makedirs(prometheus_multiproc_dir, exist_ok=True)


same as above

simonmicro · 2026-08-07T12:57:19Z

+      prometheus_multiproc_dir = '/tmp/prometheus_multiproc'
+      if not os.path.exists(prometheus_multiproc_dir):
+        os.makedirs(prometheus_multiproc_dir, exist_ok=True)
+      # Set both uppercase and lowercase variants for compatibility with different prometheus_client versions


yeah, no - you literally specified prometheus-client==0.21.0, so I do not see us being backward-compatible to a version >3 years old (a quick search turned up https://pypi.org/project/prometheus-client/0.17.1/)

please remove that

simonmicro · 2026-08-07T13:00:11Z

+
+## Overview
+
+py-kms server (python3 Docker image only) supports exporting metrics in Prometheus format for monitoring and observability. This feature is designed for testing, learning, and lab environments to better understand KMS activation protocol behavior.


I would welcome if these docs would be more concise and less flowery worded as using fillers like "This feature is designed for testing, learning, and lab environments" would need to be backed up with clear points how your design is fulfilling these words and as you just added the prometheus-client-lib...

Just leave that part out, please?

simonmicro · 2026-08-07T13:30:38Z

+    # Windows patterns
+    if 'windows' in sku_lower:
+        if 'server' in sku_lower:
+            # Extract Windows Server version
+            for year in ['2025', '2022', '2019', '2016', '2012']:
+                if year in sku_lower:
+                    return f"windows-server-{year}"
+            return "windows-server"
+        else:
+            # Extract Windows client version
+            if 'windows 11' in sku_lower or 'windows11' in sku_lower:
+                return "windows-11"
+            elif 'windows 10' in sku_lower or 'windows10' in sku_lower:
+                return "windows-10"
+            elif 'windows 8' in sku_lower or 'windows8' in sku_lower:
+                return "windows-8"
+            elif 'windows 7' in sku_lower or 'windows7' in sku_lower:
+                return "windows-7"
+            return "windows"
+
+    # Office patterns
+    elif 'office' in sku_lower:
+        for year in ['2024', '2021', '2019', '2016', '2013']:
+            if year in sku_lower:
+                return f"office-{year}"
+        return "office"


I see way too many hard-coded patterns in here, which will be a nightmare if we update the kms-db again in the future

please consider using RegEx and generic patterns instead to be at least somewhat compatible

if you are unsure about this, consider adding small test functions as well to demonstrate (and document) the logic on its own

simonmicro · 2026-08-07T13:31:47Z

+    if version_major in [4, 5, 6]:
+        return f"v{version_major}"


wtf? which major versions we support is not something to be decided in the metrics module?

simonmicro · 2026-08-07T13:32:03Z

+# Helper Functions
+# ==============================================================================
+
+def get_product_type(app_name):


same issue as below here...

simonmicro · 2026-08-07T13:33:28Z

+
+# Shared storage for activation events using shelve (cross-process safe)
+# Use PROMETHEUS_MULTIPROC_DIR if available, otherwise /tmp
+_events_dir = os.environ.get('PROMETHEUS_MULTIPROC_DIR', os.environ.get('prometheus_multiproc_dir', '/tmp'))


also here: please do not hardcode temporary file paths - especially when thinking about (god forbid) windows-support for people bypassing the container and directly invoking the server: just ask tempfile about that stuff

simonmicro · 2026-08-07T13:34:20Z

+        from prometheus_client import REGISTRY
+
+        # Check if we're in multiprocess mode (Gunicorn sets this)
+        prometheus_multiproc_dir = os.environ.get('PROMETHEUS_MULTIPROC_DIR', os.environ.get('prometheus_multiproc_dir'))


the same env-var getter over and over... you could just fetch it once on module loading, right?

simonmicro · 2026-08-07T13:49:21Z

Lol, the whole review took over an hour... This was longer than expected :/

Added feature: exposing prometheus metrics

e2ae7f8

simonmicro requested review from simonmicro and a lite review from Copilot August 7, 2026 12:47

simonmicro self-assigned this Aug 7, 2026

simonmicro added the enhancement New feature or request label Aug 7, 2026

Copilot started reviewing on behalf of simonmicro August 7, 2026 12:47 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

simonmicro requested changes Aug 7, 2026

View reviewed changes

		except Exception as e:
		return f'Error generating metrics: {e}', 500


		## Overview

		py-kms server (python3 Docker image only) supports exporting metrics in Prometheus format for monitoring and observability. This feature is designed for testing, learning, and lab environments to better understand KMS activation protocol behavior.

Uh oh!

Conversation

ahpooch commented Jul 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonmicro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonmicro commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

simonmicro left a comment •

edited

Loading