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

refactor wsgi request logging, for more context #492

Merged
merged 1 commit into from Dec 4, 2019

Conversation

bloodearnest
Copy link
Contributor

Renames WSGIResponse to TaliskerWSGIRequest, as it's not really just a response. Moves the logic of request logging/metrics into the class, so more context is available. Later changes will use this extra context to enhance sentry reports.

self.old_statsd = talisker.statsd.get_client.raw_update(
self.statsd_client)
talisker.logs.add_talisker_handler(logging.NOTSET, self.handler)
self.sentry_context.start()
Context.new()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FTR - this is a driveby UX improvement - the previous lines generates logs, moving the new context to here means those logs are not captured as part of the context.

WSGIMetric.errors.inc(**labels)

return extra


Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two methods are virtually unchanged from their module function versions.

extra={'request_id': rid}
)
else:
REQUESTS[rid] = request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now track the entire request object, rather than just the environ

@tonysimpson tonysimpson closed this Dec 4, 2019
@tonysimpson tonysimpson reopened this Dec 4, 2019
Copy link
Contributor

@tonysimpson tonysimpson left a comment

Choose a reason for hiding this comment

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

LGTM

@bloodearnest bloodearnest merged commit 10923a1 into master Dec 4, 2019
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

2 participants