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

Improve Flask integration #79

Merged
merged 5 commits into from May 8, 2018
Merged

Improve Flask integration #79

merged 5 commits into from May 8, 2018

Conversation

c-w
Copy link
Contributor

@c-w c-w commented May 3, 2018

Unlike the Django integration for Application Insights, the Flask integration currently is quite low-level: the developer has to add a custom WSGI middleware and enable trace and exception logging themselves.

A more common pattern for adding groups of new capabilities to Flask is using custom extensions that initialize themselves against the Flask application: http://flask.pocoo.org/docs/0.12/extensiondev/

As such, this commit adds a custom extension for Flask that enables a developer to very easily add Application Insights integration to their Flask application just by doing AppInsights(app).

The extension enables:

  • Request logging
  • Trace logging
  • Exception logging

Unlike the Django integration for Application Insights, the Flask
integration currently is quite low-level: the developer has to add a
custom WSGI middleware and enable trace and exception logging
themselves.

A more common pattern for adding groups of new capabilities to Flask is
using custom extensions that initialize themselves against the Flask
application: http://flask.pocoo.org/docs/0.12/extensiondev/

As such, this commit adds a custom extension for Flask that enables a
developer to very easily add Application Insights integration to their
Flask application just by doing `AppInsights(app)`.

The extension enables:
- Request logging
- Trace logging
- Exception logging
README.md Outdated
app = Flask(__name__)
app.wsgi_app = WSGIApplication('<YOUR INSTRUMENTATION KEY GOES HERE>', app.wsgi_app)
app.config['APPLICATION_INSIGHTS_KEY'] = '<YOUR INSTRUMENTATION KEY GOES HERE>'
Copy link
Contributor

Choose a reason for hiding this comment

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

Flask doesn't translate environment variables into the config automatically. So this comment is not very critical. On some environments in Azure like Web Apps instrumentation key will be set in environment variable APPINSIGHTS_INSTRUMENTATIONKEY. Name you use for config is different. Would it be more confusing to align with the name of environment variable or beneficial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done in d95fd79.

Copy link
Contributor Author

@c-w c-w May 3, 2018

Choose a reason for hiding this comment

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

Might have slightly misunderstood the original intent of the comment so I also added the option to auto-load the value from the OS environment in 157a5b7.

README.md Outdated
app.config['APPLICATION_INSIGHTS_KEY'] = '<YOUR INSTRUMENTATION KEY GOES HERE>'

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

Choose a reason for hiding this comment

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

It will also be good to configure EndpointUrl here. Endpoint URL is used for reverse proxy and compliance scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know, done in aa0d8da.

@SergeyKanzhelev
Copy link
Contributor

JJ is on the leave. @OsvaldoRosado can you please also take a look before the merge

```python
from flask import Flask
from applicationinsights.requests import WSGIApplication
from applicationinsights.flask.ext import AppInsights
Copy link
Member

Choose a reason for hiding this comment

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

Is the WSGI wrapper useful for other frameworks? We might not want to remove it from the docs.

Copy link
Contributor Author

@c-w c-w May 8, 2018

Choose a reason for hiding this comment

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

Added a link to the middleware in 98da654. Also updated the middleware's docstring to not use Flask as the code example to avoid potential confusion.

README.md Outdated
# ensure that the telemetry gets sent
@app.after_request
def send_telemetry(response):
appinsights.flush()
Copy link
Member

Choose a reason for hiding this comment

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

Is this a good practice to recommend? Won't this impede batching?

Copy link
Contributor Author

@c-w c-w May 8, 2018

Choose a reason for hiding this comment

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

Removed in 98da654.

- Add docstrings for Flask extension
- Add link to WSGIApplication in README
- Update WSGIApplication usage example to not use Flask
@c-w
Copy link
Contributor Author

c-w commented May 8, 2018

Hi @OsvaldoRosado and @SergeyKanzhelev. Thanks for the reviews. I should have addressed all the comments now. Do you have any other questions or concerns before this can be merged?

@SergeyKanzhelev SergeyKanzhelev merged commit 470c255 into microsoft:master May 8, 2018
@SergeyKanzhelev
Copy link
Contributor

@c-w thank you!

@c-w
Copy link
Contributor Author

c-w commented May 8, 2018

Thanks for the merge, @SergeyKanzhelev! Any ideas when you'll be able to do a new release to PyPI? I have a partner who's already using this new Flask integration for Application Insights and it would be great to get them onto an official dependency instead of a Github zip archive.

@SergeyKanzhelev
Copy link
Contributor

@c-w I'll try to release it tonight.

@c-w
Copy link
Contributor Author

c-w commented May 8, 2018

Awesome! Thanks a lot!

@SergeyKanzhelev
Copy link
Contributor

@c-w check it out. Just published

@c-w
Copy link
Contributor Author

c-w commented May 9, 2018

Great, thanks!

@c-w c-w deleted the flask-integration branch May 29, 2018 13:54
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.

None yet

3 participants