-
Notifications
You must be signed in to change notification settings - Fork 563
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
Add botocore instrumentation #689
Conversation
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.
Thanks for doing this work, just a couple of changes requested. Looks like there's also an entry missing in the docs.
ext/opentelemetry-ext-botocore/src/opentelemetry/ext/botocore/version.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-botocore/src/opentelemetry/ext/botocore/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-botocore/tests/test_botocore_instrumentation.py
Outdated
Show resolved
Hide resolved
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.
Thanks for addressing my comments, the code looks good. Looks like it's missing an entry in the docs.
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.
Approving although I think there's a few improvements around the data structure choices, looks good!
I wanted to raise a couple questions before we finish it up:
Merge with boto instrumentation?
A lot of code is shared with the boto instrumentation, due to what I assume is relying on data structures that boto exposes that are actually components of botocore. Does it make sense to combine instrumentations? This may help with situations like ensuring botocore instrumentation and boto instrumentation don't overlap each other.
Boto + Botocore auto-instrumentation
I think one can imperatively choose not to instrument boto / botocore depending on what level of granularity they want. But how will this work with auto-instrumentation?
Also I wonder what value instrumenting boto provides if botocore already provides a majority of the context by wrapping the _make_api_call, which I presume is what is called by the boto authenticated requests.
ext/opentelemetry-ext-botocore/src/opentelemetry/ext/botocore/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-botocore/src/opentelemetry/ext/botocore/__init__.py
Show resolved
Hide resolved
ext/opentelemetry-ext-botocore/src/opentelemetry/ext/botocore/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-ext-botocore/src/opentelemetry/ext/botocore/__init__.py
Show resolved
Hide resolved
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.
@ocelotl if you can follow up quick with the botocore / boto refactor, that would be great. Also a strategy that allows both to work in tandem with auto-instrumentation without double logging all spans.
…version.py Co-authored-by: alrex <alrex.boten@gmail.com>
Co-authored-by: alrex <alrex.boten@gmail.com>
Adding initial boto core implementation. Co-authored-by: alrex <alrex.boten@gmail.com>
* add benchmark README and latest numbers * chore: update readme chore: update readme * chore: update readme chore: update readme * chore: update benchmarks * generate latest benchmark numbers Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com> Co-authored-by: Olivier Albertini <olivier.albertini@montreal.ca>
Fixes #675