-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add common properties argument to WSGIApplication wrapper #88
Add common properties argument to WSGIApplication wrapper #88
Conversation
@@ -49,6 +49,7 @@ def __init__(self, instrumentation_key, wsgi_application, *args, **kwargs): | |||
self.client = applicationinsights.TelemetryClient(instrumentation_key, telemetry_channel) | |||
self.client.context.device.type = "PC" | |||
self._wsgi_application = wsgi_application | |||
self._common_properties = kwargs.pop('common_properties', {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SergeyKanzhelev How would you like the 'common_properties' parameter to be documented in the init documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it will be great if you can add a note into README.md. I see that DESCRIPTION.rst
is out of sync with readme. @OsvaldoRosado @jjjordanmsft do you know the history here? From looks of it I'd say readme should just point to DESCRIPTION.rst instead of duplicating the context
Can you please add an entry into changelog https://github.com/Microsoft/ApplicationInsights-Python/blob/master/CHANGELOG.md so release notes will be easier to write down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add changelog entry and a note into the readme before I'll merge
@@ -49,6 +49,7 @@ def __init__(self, instrumentation_key, wsgi_application, *args, **kwargs): | |||
self.client = applicationinsights.TelemetryClient(instrumentation_key, telemetry_channel) | |||
self.client.context.device.type = "PC" | |||
self._wsgi_application = wsgi_application | |||
self._common_properties = kwargs.pop('common_properties', {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it will be great if you can add a note into README.md. I see that DESCRIPTION.rst
is out of sync with readme. @OsvaldoRosado @jjjordanmsft do you know the history here? From looks of it I'd say readme should just point to DESCRIPTION.rst instead of duplicating the context
I'd say just update README.md for now and while we ought to address the duplication, let's do so in another issue (#89). |
@SergeyKanzhelev @jjjordanmsft |
@@ -1,9 +1,10 @@ | |||
# Changelog | |||
|
|||
## 0.11.5 | |||
- Add common properties argument to WSGIApplication initialization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't shipped 11.4
yet. So it should be reported under previous title. I'll merge and fix this
Our Flask app cannot log common properties/dimensions when it uses the default WSGIApplication wrapper. I have updated the wrapper construct to take in a new common_properties object. This is similar to the code used in the NodeJS Application Insight Telemetry Logger: https://github.com/Microsoft/ApplicationInsights-node.js/blob/develop/Library/TelemetryClient.ts#L36
These common properties are then logged for every track_request made by the WSGIApplication wrapper.