-
Notifications
You must be signed in to change notification settings - Fork 45
Statsd integration #89
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
Conversation
datadog_lambda/metric.py
Outdated
|
||
class StatsDWrapper: | ||
""" | ||
Wraps StatsD calls, to give an identical to ThreadStats |
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.
to give an identical what?
tests/test_wrapper.py
Outdated
self.addCleanup(patcher.stop) | ||
# patcher = patch("datadog_lambda.metric.lambda_metric") | ||
# self.mock_lambda_metric = patcher.start() | ||
# self.addCleanup(patcher.stop) |
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.
What's this?
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 were patching the datadog_lambda.metric.lambda_metric out for testing, but the new code uses it internally, so I unpatched it. I'll remove this snippet.
datadog_lambda/extension.py
Outdated
return True | ||
|
||
|
||
use_extension = is_extension_running() |
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.
To me, use_extension
sounds like a verb, i.e. if I call it, then the extension will be used. But it seems like it represents whether the extension should be used. Maybe rename this to is_using_extension
or should_use_extension
or something along those lines?
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.
Lgtm, just a few nits
What does this PR do?
Supports the extension via statsd