use the init_app style for use with app factory pattern #8

hoopes · 2018-09-11T20:29:59Z

Hey,

I'd love to use this in my flask app, but we use the "app factory" pattern. http://flask.pocoo.org/docs/0.12/extensiondev/#the-extension-code shows an example of this. I tried to do two things here:

Allow no app object to be passed to the constructor.
Use current_app instead of self.app (See the "Note on init_app" near the bottom of that linked section)

The **kwargs basically copying the constructor args might be a little awkward, but is there for backwards compatibility? Maybe? You could also scrap that, and just leave the constructor args, and init_app just take the app object.

In any case, if you're into this, or would like some changes, please let me know.

coveralls · 2018-09-11T20:31:50Z

Coverage decreased (-0.6%) to 95.798% when pulling 4bbddc1 on hoopes:init-app-style into cea8ce4 on rycus86:master.

rycus86 · 2018-09-11T21:06:58Z

Hi @hoopes !

Thanks for the contribution, sounds like a good idea!
I haven't used this pattern yet, so it might be good if we could have some tests for this?

Also, can you please look at Python 3.x compatibility?
Looks like it's broken at the moment: https://travis-ci.org/rycus86/prometheus_flask_exporter/jobs/427394362

Thank you!

hoopes · 2018-09-11T21:08:27Z

Yeah, I will be glad to. Also, proving my theory that every time i do something public on the internet, I look like an idiot, I didn't even test thoroughly enough "by hand" - it's not working properly. I will add some automated tests as well.

rycus86 · 2018-09-11T21:16:04Z

Oh no, you're all right, and I'm thankful for your interest in the project! :)
Hopefully automated test will give you some help with the issues, but let me know if you need a hand with it!

Thanks again!

hoopes · 2018-09-12T02:15:46Z

Am I right or wrong in assuming the Cognitive Complexity has gone to 6 because there's another argument in the export_defaults method? I don't want to even attempt to refactor that method, but do you know another way to get below that threshold? Thanks!

Also - the test I added just checks that calling init_app instead of using the constructor still makes successful calls. Is there something else you'd like to see? Using the app factory pattern shouldn't change much about how the extension acts.

rycus86 · 2018-09-12T16:34:23Z

I can imagine that is the reason, yes. I wouldn't worry about it too much. Let me try it in the evening, then I'll merge it in.

Thanks again!

rycus86 · 2018-09-13T20:26:18Z

Sorry for the delay @hoopes , I added some comments now. The main thing is about not attaching the app object if it was passed in to the constructor, it would be great if you could resolve that. The documentation part I can take care of, unless you feel like taking care of it. :)

Also, could you potentially squash this down to a single commit when you're done?

Thanks a lot!

hoopes · 2018-09-14T13:02:22Z

prometheus_flask_exporter/__init__.py

+            'registry' : registry,
+        }
+
+        if app:


@rycus86 This will call init_app if the app is passed into the constructor. Is that what you meant?

Hey, no, sorry if I wasn't clear. We used to assign self.app as the app passed in to the constructor. With your changes, we don't anymore. This is a problem in the register_endpoint and export_defaults functions, when we use the original logic, as it now doesn't have that reference - which is why the extra app_context().push() was necessary in the test setup.
If we hold the reference to the app passed in to the constructor, we can restore the original logic by using this in those functions:

if app is None: app = self.app or current_app

maybe i should have tested more thoroughly added a test showing that the app factory pattern also works reducing my cognitive complexity, maybe added small readme blurb about the app factory pattern

hoopes · 2018-09-14T18:43:35Z

Commit has been squashed. Not sure if you wanted me to bump the version number as well, but I thought you could handle that with however you do your releases.

rycus86 · 2018-09-14T19:54:08Z

Thanks for the changes @hoopes ! I'll take care of the version numbers, don't worry about that bit.
Can you please have a look through my comments on the changed files here: https://github.com/rycus86/prometheus_flask_exporter/pull/8/files

Thanks!

hoopes · 2018-09-14T20:14:38Z

Hey, I must be missing something obvious, but I don't see any comments - just my one comment about the init_app in the constructor.

rycus86 · 2018-09-13T20:16:38Z

prometheus_flask_exporter/__init__.py

@@ -66,7 +66,7 @@ def echo_status(status):
        - Without an argument, possibly to use with the Flask `request` object
    """

-    def __init__(self, app, path='/metrics',
+    def __init__(self, app=None, path='/metrics',


I wonder whether it would be more sensible to leave app as a required parameter, and having to pass in None when you want to do the app factory pattern, so that the common (?) way of doing this is more relevant - if that makes sense.

rycus86 · 2018-09-13T20:17:23Z

prometheus_flask_exporter/__init__.py

@@ -82,16 +82,35 @@ def __init__(self, app, path='/metrics',
            (will use the default when `None`)
        :param registry: the Prometheus Registry to use
        """
+        self._options = {


I'm not sure I see the benefit of doing it this way, we could just pass the necessary parameters to init_app, which I would also change from **kwargs parameters to explicit parameters.

rycus86 · 2018-09-13T20:18:50Z

tests/test_app_factory.py

+
+class AppFactoryTest(BaseTestCase):
+
+    def metrics_init(self, **kwargs):


I'd probably rename this to setUp, and just not use the BaseTestCase. We could just duplicate the (minimal) required test setup here.

rycus86 · 2018-09-13T20:19:40Z

tests/unittest_helper.py

@@ -17,6 +17,7 @@ def __init__(self, *args, **kwargs):
    def setUp(self):
        self.app = Flask(__name__)
        self.app.testing = True
+        self.app.app_context().push()


This shouldn't be necessary for the existing tests. I'll add some comments on how I managed to solve this locally.

rycus86 · 2018-09-13T20:22:34Z

prometheus_flask_exporter/__init__.py

@@ -105,8 +124,7 @@ def register_endpoint(self, path, app=None):
        if is_running_from_reloader():
            return

-        if app is None:
-            app = self.app
+        app = app or current_app


To keep the previous logic too, we should keep a reference to the app passed in the constructor (but not in the init_app function, then this block should change to:

if app is None: app = self.app or current_app

rycus86 · 2018-09-13T20:22:57Z

prometheus_flask_exporter/__init__.py

@@ -162,6 +180,9 @@ def export_defaults(self, buckets=None, group_by_endpoint=False):
            by the endpoints' function name instead of the URI path
        """

+        if not app:
+            app = current_app


Like I mentioned above, this block should change to:

if app is None: app = self.app or current_app

rycus86 · 2018-09-13T20:24:03Z

prometheus_flask_exporter/__init__.py

+
+    def init_app(self, app, **kwargs):
+        """
+        See the arguments for __init__ for more info.


We'll probably need more explanation on what is this function for, and how to use it, plus perhaps some changes to the comments in __init__. Although I'm happy to change these and the README after this PR is merged if you prefer.

rycus86 · 2018-09-14T19:53:07Z

prometheus_flask_exporter/__init__.py

+            'registry' : registry,
+        }
+
+        if app:


Hey, no, sorry if I wasn't clear. We used to assign self.app as the app passed in to the constructor. With your changes, we don't anymore. This is a problem in the register_endpoint and export_defaults functions, when we use the original logic, as it now doesn't have that reference - which is why the extra app_context().push() was necessary in the test setup.
If we hold the reference to the app passed in to the constructor, we can restore the original logic by using this in those functions:

if app is None: app = self.app or current_app

rycus86 · 2018-09-14T20:20:57Z

Hey, I must be missing something obvious, but I don't see any comments - just my one comment about the init_app in the constructor.

🤦‍♂️ sorry, first time requesting changes here... Apparently I need to press a button, not just leave comments. Sorry again, hopefully you should see them now?

hoopes · 2018-09-14T20:23:14Z

Got it, ok, i'll try to handle this weekend. Thanks!

hoopes · 2018-09-17T18:47:55Z

Hey, let me know if you're more into this style. I tried to steal the app factory details from flask-sqlalchemy.

https://github.com/mitsuhiko/flask-sqlalchemy/blob/master/flask_sqlalchemy/__init__.py#L675

Note that i needed to use self._export_defaults as the instance var, b/c export_defaults is also a method. Not sure if you have ideas on how you wanna handle that.

rycus86 · 2018-09-20T09:01:41Z

Hi @hoopes ,

Sorry, I was unavailable for a few days. Thanks for the changes!
There are a couple of nits here and there, but overall looks OK.
Let me merge it in, change those little bits around (mainly how the self.app is managed), then I'll push a new release.

Thanks for you work on this!

rycus86 · 2018-09-20T09:29:15Z

And 0.3.0 should now be available: https://pypi.org/project/prometheus-flask-exporter/0.3.0/

hoopes · 2018-09-20T12:58:54Z

Thanks man, this was fun. Appreciate all your help.

hoopes commented Sep 14, 2018

View reviewed changes

use the init_app style for use with app factory pattern

183368f

maybe i should have tested more thoroughly added a test showing that the app factory pattern also works reducing my cognitive complexity, maybe added small readme blurb about the app factory pattern

hoopes force-pushed the init-app-style branch from b6efa2a to 183368f Compare September 14, 2018 13:33

rycus86 requested changes Sep 14, 2018

View reviewed changes

address comments from PR

4bbddc1

rycus86 merged commit 6b0c70c into rycus86:master Sep 20, 2018

hoopes deleted the init-app-style branch September 20, 2018 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use the init_app style for use with app factory pattern #8

use the init_app style for use with app factory pattern #8

hoopes commented Sep 11, 2018

coveralls commented Sep 11, 2018 •

edited

Loading

rycus86 commented Sep 11, 2018

hoopes commented Sep 11, 2018 •

edited

Loading

rycus86 commented Sep 11, 2018

hoopes commented Sep 12, 2018

rycus86 commented Sep 12, 2018

rycus86 commented Sep 13, 2018

hoopes Sep 14, 2018

rycus86 Sep 14, 2018

hoopes commented Sep 14, 2018

rycus86 commented Sep 14, 2018

hoopes commented Sep 14, 2018

rycus86 Sep 13, 2018

rycus86 Sep 13, 2018

rycus86 Sep 13, 2018

rycus86 Sep 13, 2018

rycus86 Sep 13, 2018

rycus86 Sep 13, 2018

rycus86 Sep 13, 2018

rycus86 Sep 14, 2018

rycus86 commented Sep 14, 2018

hoopes commented Sep 14, 2018

hoopes commented Sep 17, 2018

rycus86 commented Sep 20, 2018

rycus86 commented Sep 20, 2018

hoopes commented Sep 20, 2018


		class AppFactoryTest(BaseTestCase):

		def metrics_init(self, **kwargs):

use the init_app style for use with app factory pattern #8

use the init_app style for use with app factory pattern #8

Conversation

hoopes commented Sep 11, 2018

coveralls commented Sep 11, 2018 • edited Loading

rycus86 commented Sep 11, 2018

hoopes commented Sep 11, 2018 • edited Loading

rycus86 commented Sep 11, 2018

hoopes commented Sep 12, 2018

rycus86 commented Sep 12, 2018

rycus86 commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoopes commented Sep 14, 2018

rycus86 commented Sep 14, 2018

hoopes commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rycus86 commented Sep 14, 2018

hoopes commented Sep 14, 2018

hoopes commented Sep 17, 2018

rycus86 commented Sep 20, 2018

rycus86 commented Sep 20, 2018

hoopes commented Sep 20, 2018

coveralls commented Sep 11, 2018 •

edited

Loading

hoopes commented Sep 11, 2018 •

edited

Loading