opentelemetry decorator #6
Conversation
There was a problem hiding this comment.
The inspect based improvements in that last commit are really nice, great stuff!
However, I'm not sure about creating a separate dependency for this at this stage. I'd be tempted to let it bake in airlock for a bit before extracting, rather than having an extra dep?
Or are you thinking to release this on pypi as a general open source tool?
| attributes_dict = attributes or {} | ||
| if func_attributes is not None: | ||
| func_signature = signature(func) | ||
| bound_args = func_signature.bind(*args, **kwargs).arguments |
There was a problem hiding this comment.
Huh, TIL. This is really quite nice!
otel_opensafely/trace.py
Outdated
| # 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) | ||
| if param and param.default is not Parameter.empty: |
|
@bloodearnest So, I pulled this out because of your comment about using it in job-server and job-runner too, and I didn't want to copy/paste the decorator code there. But maybe we're best to leave it in just airlock for a while first? I can just update airlock with the changes from the last commit and leave the separate package for now. It's complaining about dependencies on different python versions, and I think job-runner's still on 3.8. There's nothing opensafely-specific if we were to make it a public package, but it'd probably need a better name. |
Ah, sorry, yes, I meant it "at some point"! But the improvements from working on it are great.
I just saw and reviewed your PR, nice.
Agreed, I think it might be worth doing the work to release to the world, but maybe it can wait until post-initiative deck scrubbing? |
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.
Pulling the @Instrument decorator from airlock into its own package so we can use it elsewhere.
The last commit does some refactoring and makes some improvements to how it finds attributes from function args.