Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use local vars, not instance vars, in metricsHandler #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmazin
Copy link

@dmazin dmazin commented May 24, 2024

When Falcon starts up, it creates an individual instance of metricsHandler for each metric type:

def falcon_app():
    api = falcon.API()
    api.add_route("/health", metricsHandler(config, metrics_type="health"))

The same metricsHandler instance is used for every /health request.

This, by itself, is not an issue. However, because the instance stores state (such as IP/hostname) in instances variables, these can get clobbered if we process multiple requests simultaneously.

By instance variables I mean usage of self:

class metricsHandler:
    ...
    def on_get(self, req, resp):
        self.target = req.get_param("target")

Here is you can reproduce this.
First, let's add some logging to show the issue.

diff --git a/collector.py b/collector.py
index 32d29d9..e183ddc 100644
--- a/collector.py
+++ b/collector.py
@@ -20,6 +20,8 @@ class RedfishMetricsCollector(object):
        self.target = target
        self.host = host

+       logging.info(f"**** Initializing RedfishMetricsCollector for hostname {self.host} and IP {self.target}")
+
        self._username = usr
        self._password = pwd
diff --git a/handler.py b/handler.py
index b035449..1627ffc 100644
--- a/handler.py
+++ b/handler.py
@@ -33,6 +33,8 @@ class metricsHandler:
        self.metrics_type = metrics_type

    def on_get(self, req, resp):
+       logging.info(f"**** The id of this metricsHandler instance is {id(self)}")
+
        self.target = req.get_param("target")
        if not self.target:
            logging.error("No target parameter provided!")
@@ -87,6 +89,7 @@ class metricsHandler:
        logging.debug(f"{self.target}: Using user {usr}")

+      logging.info(f"**** Scraping {self.host}, the client asked for {req.get_param('target')}")
        with RedfishMetricsCollector(
            self._config,
            target = self.target,

I am going to use the following bash script to quickly make two requests against the exporter:

#!/bin/bash
idracs=("foo" "bar")
for idrac in "${idracs[@]}"; do
    curl "http://localhost:9229/health?target=${idrac}&job=redfish" -o "${idrac}_stats.txt" &
done

Now, let’s observe the logs:

handler.py:36 INFO  *** The id of this metricsHandler instance is 131570941555444
handler.py:36 INFO  *** The id of this metricsHandler instance is 131570941555444
handler.py:94 INFO  *** Scraping foo; the client asked for foo
handler.py:94 INFO  *** Scraping foo; the client asked for bar
collector.py:23 INFO  *** Initializing RedfishMetricsCollector for hostname foo and IP 10.0.0.3.
collector.py:23 INFO  *** Initializing RedfishMetricsCollector for hostname foo and IP 10.0.0.3.

We see a few things:
• Lines 1,2: Indeed, the very same metricsHandler instance processes both requests (this is evidenced by the id being identical). Again, this isn’t an issue unto itself; just saying it’s the same instance so we can’t use self safely.
• Lines 3,4: You can see that it scrapes foo even though the client asked for bar
• Lines 5,6: From collector.py, you can see that the collector is initialized for foo and IP 10.0.0.3 twice even though we had asked for bar on the second request.

fixes #9

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.

Thread safety issue?
1 participant