-
Notifications
You must be signed in to change notification settings - Fork 0
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 the instrument decorator to use inspect to find arguments #192
Conversation
a995283
to
f9cf8d8
Compare
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.
Looks good.
Have left some comments, this is a fun bit of python wrangling!
""" | ||
|
||
def span_decorator(func): | ||
tracer = existing_tracer or trace.get_tracer("airlock") | ||
tracer = existing_tracer or trace.get_tracer( | ||
os.environ.get("OTEL_SERVICE_NAME", "airlock") |
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.
Nice touch
services/tracing.py
Outdated
@@ -93,24 +101,19 @@ def _set_attributes(span, attributes_dict): | |||
@wraps(func) | |||
def wrap_with_span(*args, **kwargs): | |||
name = span_name or func.__qualname__ | |||
attributes_dict = attributes or {} | |||
if func_attributes is not None: | |||
func_signature = signature(func) |
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.
Can we move this line up into the decorator body? Then its called once at import time, rather than on every func invocation?
services/tracing.py
Outdated
return func_bound_args[parameter_name] | ||
# an argument for this parameter was not passed explicitly in the function call; | ||
# look for a default value | ||
param = func_signature.parameters.get(parameter_name) |
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.
Can we extract default parameters in the decorator body at import time into a dict, and then just look them up at call time?
Combined with pulling out the signature in the decorator at import time, we can build the default param dict then too.
If so, this function might be small enough to not bother with, we can just inline it in the loop, as we can just use ChainMap to look up the parameter in a series of dicts:
(Note, this also include removing the need for _set_attributes, as per my other comment)
...
from collections import ChainMap
...
def instrument(...):
...
def span_decorator(func):
...
func_signature = signature(func)
default_args = {
k: v.default for k, v in func_signature.parameters.items()
if v.default != inspect.Parameter.empty,
}
bound_args = {}
@wraps(func)
def wrap_with_span(*args, **kwargs):
...
if func_attributes is not None:
bound_args = func_signature.bind(*args, **kwargs).arguments
param_lookup = ChainMap(kwargs, bound_args, default_args)
for k, v in func_attributes.items():
try:
attributes_dict[k] = str(param_lookup[v])
except ...:
...
with tracer.start_as_current_span(
name, record_exception=record_exception, attributes=attribute_dict,
):
return func(*args, **kwargs)
return wrap_with_span
services/tracing.py
Outdated
func_signature, bound_args, kwargs, v | ||
) | ||
attributes_dict[k] = str(func_arg) | ||
|
||
with tracer.start_as_current_span( | ||
name, record_exception=record_exception |
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.
You can pass attributes at span creation, so we could maybe do
with tracer.start_as_current_span(name, record_exception=record_exception, attributes=attributes_dict):
This would remove the need for _set_attributes alltogether?
By using the inspect module to identify the bound arguments passed to the function, we don't need to separate arg and kwarg attributes, and we can handle cases different types of function call, where keywords aren't enforced. We can also now handle keywords captured in a **kwargs parameter.
f9cf8d8
to
5ea9947
Compare
@bloodearnest Have implemented all your suggestions, much cleaner now I think. Thanks! |
By using the inspect module to identify the bound arguments passed to the function, we don't need to separate arg and kwarg attributes, and we can handle cases different types of function call, where keywords aren't enforced. We can also now handle keywords captured in a **kwargs parameter.