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

Feature request: Logging instance on Api/Namespace object #694

Closed
casparjespersen opened this issue Aug 20, 2019 · 11 comments
Closed

Feature request: Logging instance on Api/Namespace object #694

casparjespersen opened this issue Aug 20, 2019 · 11 comments

Comments

@casparjespersen
Copy link

First: Thanks for a great project. Flask has an out-of-the-box logger module attached on the Flask object, that you can interact with the following:

from flask import Flask
app = Flask(__name__)

@app.route("/", methods=["GET"])
def index():
    app.logger.info("Received index load")
    return "Hello, world!"

This has the advantage, that for instance when using application insights, you can simply attach to that application:

from applicationinsights.flask.ext import AppInsights

# instantiate the Flask application
app = Flask(__name__)
app.config['APPINSIGHTS_INSTRUMENTATIONKEY'] = '<YOUR INSTRUMENTATION KEY GOES HERE>'

# log requests, traces and exceptions to the Application Insights service
appinsights = AppInsights(app)

With that in mind, I think it would be very nice to expose this logger on the Api or Namespace object:

from flask_restplus import Namespace, Api

foo_api = Namespace("api", path="/api")

@foo_api.route("/foo")
class Foo(Resource):
    def get(self):
        foo_api.logger.info("Received foo load") # or self.logger...

And then this should either create its own logging instance, or attach to the Flask one when connected to an Api:

from flask import Flask
from flask_restplus import Api
from foobar import foo_api

app = Flask(__name__)
api = Api(app=app)
api.add_namespace(foo_api)
# this will attach the logging instance from `foo_api` through `api` to `app`

Thoughts?

@SteadBytes
Copy link
Collaborator

This is a nice idea, have you any thoughts regarding how logging levels should be managed across different Api/Namespaces. For example, the log level could be set for each Namespace:

foo_api = Namespace("api", path="/api", log_level=DEBUG)
bar_api = Namespace("api", path="/api", log_level=WARNING)

However, these levels should be overridden by the top level Api they are attached to:

app = Flask(__name__)
api = Api(app=app, log_level=ERROR)
api.add_namespace(foo_api)
api.add_namespace(bar_api)

The logs from a given Namespace will then only be seen if the log level on the top level Api is <= that of the Namespace. What do you think?

@casparjespersen
Copy link
Author

I haven't worked extensively with the logging module, so I'm not fully aware of the capabilities. For now, however, I'll assume that it's somehow possible to forward calls from one logging instance to another. In that case, I see two ways forward:

Option A: Simple
Since a Namespace is attached to an Api, and an Api is attached to a Flask, and Flask has a logger instance. We can simply expose a logger through the Namespace, but forward it directly to the logger of the Flask instance at runtime. That would provide a global out-of-the-box logger. This, I think, is the MVP.

Option B: Advanced
We can also go in a direction with more control, as you're suggesting. I like the idea, but maybe we can generalize it a bit more? Logging level is not the only argument of interest for the user. If we instead expose the logger instance on Namespace, we can leave it to the user to do whatever:

foo_api = Namespace("foo", path="/foo")
foo_api.logger.set_level(DEBUG)
bar_api = Namespace("bar", path="/bar")
bar_api.logger.set_level(INFO)
bar_api.logger.set_format(...)

app = Flask(__name__)
api = Api(app=app)
api.add_namespace(foo_api)
api.add_namespace(bar_api)

@SteadBytes
Copy link
Collaborator

The Python logging module is very extensible, we can add the handlers of a logger to that of another - the Python Logging Cookbook has some examples.

I was throwing out some ideas to try and ascertain the intended API for the feature to then work out the implementation details, however IMO your Option B is very nice. It allows the flexibility of having different loggers for each Api/Namespace and keeps them configurable without deviating from the builtin logging module API. That will make it easy to use for anyone who has used the Python logging module before (hopefully most people 😄 ).

Option B is the way to go IMO - what do you think?

@casparjespersen
Copy link
Author

I agree - option B seems appealing. I can give it an initial shot in the near future and submit a PR.

@SteadBytes
Copy link
Collaborator

Great, I'm happy to work on the implementation (I'm a bit of a logging geek 😄 ) if you would rather that. Also happy for you to do it if you're keen of course! I'll review/help throughout 👍

@casparjespersen
Copy link
Author

Great, I'm happy to work on the implementation (I'm a bit of a logging geek 😄 ) if you would rather that. Also happy for you to do it if you're keen of course! I'll review/help throughout 👍

Haha. If you're up for it - give it a shot. Being a self-declared logging geek, I think you should have the opportunity 😉

@SteadBytes
Copy link
Collaborator

SteadBytes commented Aug 25, 2019

Ok great thank you, I'm not sure whether it's a good thing to be a logging geek now I've said it 😉 We do encourage community contribution for Flask-RESTPlus so if you take a look at my implementation and think you've got a better way to do it then by all means please do go ahead! 😄 Since this is your feature request, I will ask for your review (as well as another maintainer) on the PR if that's ok ?

Here's my interpretation of option B - please provide suggestions/improvements where you see fit 😄

Each Namespace will have it's own unique logging.Logger instance, initialised with the name of the Namespace.

foo_api = Namespace("foo", path="/foo")
bar_api = Namespace("bar", path="/bar")

>>> foo_api.logger.name
'flask_restplus.namespace.foo'
>>> bar_api.logger.name
'flask_restplus.namespace.bar'

These loggers will by default inherit the configuration of the Flask app object logger (log levels, handlers e.t.c), unless explicitly configured otherwise:

app = Flask(__name__)
# set app logger level
logging.basicConfig(level=logging.INFO)

api = Api(app=app)

foo_api = api.namespace("foo", path="/foo")
bar_api = api.amespace("bar", path="/bar")
# add additional handler for bar_api only
handler = logging.FileHandler("/path/to/log/file")
handler.setFormatter(logging.Formatter('%(name)s - %(levelname)s - %(message)s'))
bar_api.addHandler(handler)

baz_api = api.namespace("baz", path="/bar")
# override app log level for baz_api only
baz_api.logger.setLevel(logging.DEBUG)


@foo_api.route('/foo')
class Foo(Resource):
    def get(self):
        foo_api.logger.debug("hello from foo") # not logged
        return {"message": "foo"}


@bar_api.route('/bar')
class Bar(Resource):
    def get(self):
        bar_api.logger.info("hello from bar") # logged to app handlers and custom file handler
        return {"message": "bar"}


@baz_api.route('/baz')
class Baz(Resource):
    def get(self):
        bar_api.logger.debug("hello from baz")  # logged to app handlers
        return {"message": "bar"}

Does that seem like a good API for this feature? I'm not entirely sure on the desired behaviour for overriding/inheriting log configuration but it seems that using the Flask logger as the default configuration for each Namespace and then explicitly overriding where desired makes sense. The other option would be to not do that at all and simply create the loggers, but then leave it up to the user to configure them as they wish. Some other Flask extensions take this approach where ,by default, the logs won't actually show up anywhere because the loggers don't have handlers and it's up to the user to add the handlers (often hooking them up to the Flask logger).

In the issue you mentioned application insights, I haven't used this before so I'm wondering if there is any specific information/hooks that need to be exposed/provided in order for it to instrument the logging - do you know of anything like this? Or is it a simple case that it will just grab all the logs as long as it's using a Python logging module logger?

@casparjespersen
Copy link
Author

Sure thing, I'll be happy to review the PR! Overall, I agree with your suggestion. To answer your questions:

I'm not entirely sure on the desired behaviour for overriding/inheriting log configuration

I think it makes most sense defaulting to the configuration of the Flask app logger on each namespace logger, and then, as you suggest, the user can always customize once they are exposed.

In the issue you mentioned application insights, I haven't used this before so I'm wondering if there is any specific information/hooks that need to be exposed/provided in order for it to instrument the logging - do you know of anything like this?

It appears that they add a custom handler to the Flask app.logger instance. I'm not sure this would work out-of-the-box with our current proposal? Not saying that's the goal, anyway. Would the client need to iterate over all namespace loggers, or is it possible to "bundle" them by using the app.logger?

@SteadBytes
Copy link
Collaborator

Great thank you!

The app insights logger should (in theory) work with the current proposal as long as it is added to the Flask app.logger before the namespace loggers are registered. I intended to do exactly as you said, for each namespace logger set it's configuration based on app.logger which would include all of its handlers. I'll give it a go and see how it works 👍

@SteadBytes
Copy link
Collaborator

@casparjespersen I've got a draft PR up with an initial implementation #708, it's incomplete (hence the TODO list) but it's enough to cover the behaviour we've discussed here. Would you mind taking a look and seeing if it matches up to your interpretation of what we've discussed? 😄 Thanks!

@casparjespersen
Copy link
Author

casparjespersen commented Sep 3, 2019

Sure. I'll take a look at it tomorrow, and continue the implementation discussions on the PR.

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

No branches or pull requests

2 participants