Skip to content

Conversation

savathoon
Copy link
Contributor

Using this as a sanity check for our generic metrics plugin interface for compatibility with other metric reporting libraries

@savathoon savathoon marked this pull request as ready for review July 14, 2025 17:27
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a demonstration Prometheus metrics reporter plugin to validate compatibility with the generic metrics plugin interface. The implementation provides a complete Prometheus integration with support for counters, gauges, histograms, and summaries.

Key changes include:

  • Complete Prometheus metrics reporter plugin implementation with thread-safe batching and automatic label management
  • Plugin packaging configuration with setuptools entry points and dependency management
  • Comprehensive documentation covering installation, configuration, and usage examples

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

File Description
setup.py Plugin packaging configuration with dependencies and entry points
requirements.txt Runtime dependencies for pluggy and prometheus_client
metrics_reporter.py Core Prometheus metrics reporter implementation with batching and label support
README.md Complete documentation for installation, configuration, and usage

# Ensure directory exists
os.makedirs(multiproc_dir, exist_ok=True)
# Set multiprocess mode
prometheus_client.values.ValueClass = prometheus_client.values.MultiProcessValue()
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MultiProcessValue() is being called as a function, but it should be assigned as a class. This should be prometheus_client.values.ValueClass = prometheus_client.values.MultiProcessValue (without parentheses).

Suggested change
prometheus_client.values.ValueClass = prometheus_client.values.MultiProcessValue()
prometheus_client.values.ValueClass = prometheus_client.values.MultiProcessValue

Copilot uses AI. Check for mistakes.

@@ -0,0 +1,2 @@
pluggy==1.4.0
prometheus_client>=0.17.0 No newline at end of file
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is trailing whitespace at the end of this line that should be removed.

Suggested change
prometheus_client>=0.17.0
prometheus_client>=0.17.0

Copilot uses AI. Check for mistakes.

_prometheus_metrics["counters"][prometheus_name] = Counter(
prometheus_name,
f"Counter metric for {metric_name}",
labelnames=list(labels.keys()) if labels else [],
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The labelnames parameter should be consistent across all instances of the same metric. If a metric is created with certain labelnames, all subsequent uses must have the same labelnames. Consider determining all possible labels upfront or using a different approach to handle dynamic labels.

Suggested change
labelnames=list(labels.keys()) if labels else [],
labelnames=self.metric_labelnames.setdefault(
prometheus_name, list(labels.keys()) if labels else []
),

Copilot uses AI. Check for mistakes.

env = os.environ.get("FLASK_ENV", "development")
env_tag = "prd" if env == "production" else "stg" if env == "staging" else "dev"
merged_tags["env"] = env_tag
merged_tags["service"] = "access"
Copy link
Preview

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The hardcoded service name 'access' should be configurable or derived from environment variables to make the plugin more reusable across different services.

Suggested change
merged_tags["service"] = "access"
merged_tags["service"] = os.environ.get("SERVICE_NAME", "access")

Copilot uses AI. Check for mistakes.

@somethingnew2-0
Copy link
Collaborator

Prometheus metrics work by having an external process scrape an exposed http endpoint (eg. /metrics) that outputs the metrics. For this plugin to work, it would require the Access Flask app to expose some sort of metrics route for the Prometheus server to scrape.

The Prometheus client_python documentation sorta shows how this works in their example code with start_http_server https://prometheus.github.io/client_python/

@savathoon
Copy link
Contributor Author

Prometheus metrics work by having an external process scrape an exposed http endpoint (eg. /metrics) that outputs the metrics. For this plugin to work, it would require the Access Flask app to expose some sort of metrics route for the Prometheus server to scrape.

The Prometheus client_python documentation sorta shows how this works in their example code with start_http_server https://prometheus.github.io/client_python/

🤔 we could set an env variable to configure that endpoint if prometheus. I mostly used this + the otel client lib to make sure the functions I was exposing via pluggy hook impl were generic enough to work for most metrics library options. Happy to reconfigure as just an endpoint that can be scraped by an external client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants