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

fix access logging in gaiohttp worker #1193

Merged
merged 1 commit into from Feb 3, 2016

Conversation

urbaniak
Copy link
Contributor

Access logs were wrongly formatted when using gaiohttp worker

It also fixes the statsd instrumentation with this type of worker.

@@ -237,7 +237,10 @@ def log(self, lvl, msg, *args, **kwargs):
def atoms(self, resp, req, environ, request_time):
""" Gets atoms for log formating.
"""
status = resp.status.split(None, 1)[0]
if type(resp.status) is int:
Copy link
Owner

Choose a reason for hiding this comment

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

I would do :

status = resp.status
if isinstance(status, str): 
     status = status.split(None, 1)[0]

to be consistent with the style in gunicorn. Thoughts?

@benoitc
Copy link
Owner

benoitc commented Jan 28, 2016

Good catch! Thanks.

Modulo the style bikesheeding, the patch looks fine for me. If you can do the change that would help :)

@urbaniak
Copy link
Contributor Author

I've fixed the formatting.

Found also one more issue, which should be fixed in urbaniak@4f810ee

Let me know if that's appropriate for other workers.

@urbaniak urbaniak force-pushed the gaiohttp-logging branch 3 times, most recently from 25eca42 to 3711228 Compare January 28, 2016 09:52
@urbaniak
Copy link
Contributor Author

Thought about that, done it that way because req_headers was written like that.

Should be good to go now ;)

@@ -93,9 +93,14 @@ def access(self, resp, req, environ, request_time):
"""
Logger.access(self, resp, req, environ, request_time)
duration_in_ms = request_time.seconds * 1000 + float(request_time.microseconds) / 10 ** 3
if type(resp.status) is int:
Copy link
Owner

Choose a reason for hiding this comment

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

style should also be changed there :)

@benoitc
Copy link
Owner

benoitc commented Jan 28, 2016

@urbaniak thanks! Once last change and we are good 👍

@urbaniak urbaniak force-pushed the gaiohttp-logging branch 2 times, most recently from 7875dd9 to 5dfb627 Compare January 28, 2016 11:14
@tilgovi
Copy link
Collaborator

tilgovi commented Feb 2, 2016

@urbaniak would you mind squashing the commits, please? You can just force push and keep the same PR.

@urbaniak
Copy link
Contributor Author

urbaniak commented Feb 2, 2016

No prob, squashed.

Hope to get that released soon.

@tilgovi
Copy link
Collaborator

tilgovi commented Feb 3, 2016

Thanks!

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