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

Add boto instrumentation #665

Merged
merged 27 commits into from
Jun 2, 2020
Merged

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented May 8, 2020

Fixes #628

@ocelotl ocelotl added the instrumentation Related to the instrumentation of third party libraries or frameworks label May 8, 2020
@ocelotl ocelotl requested a review from a team May 8, 2020 22:05
@ocelotl ocelotl self-assigned this May 8, 2020
@codeboten codeboten added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label May 14, 2020
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

This looks pretty good, only change required is the versions in setup.cfg and usage documentation. Looks like an entry in the docs is missing as well.

ext/opentelemetry-ext-boto/setup.cfg Outdated Show resolved Hide resolved
ext/opentelemetry-ext-boto/setup.cfg Outdated Show resolved Hide resolved
ext/opentelemetry-ext-boto/setup.cfg Outdated Show resolved Hide resolved
span.set_attribute(key, value)


def flatten_dict(dict_, sep=".", prefix=""):
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this code could be pulled out into a utility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't think this code belongs anywhere outside of this package, but if other related instrumentations use it they could import it from here.

tox.ini Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing my comments!

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

LGTM! A couple comments but non-blocking.

ext/opentelemetry-ext-boto/setup.py Outdated Show resolved Hide resolved

def truncate_arg_value(value, max_len=1024):
"""Truncate values which are bytes and greater than ``max_len``.
Useful for parameters like "Body" in ``put_object`` operations.
Copy link
Member

Choose a reason for hiding this comment

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

no need to investigate too deeply on this, but how about strings? is it possible for giant payloads to be sent as strings that should also be truncated?

maybe a json payload? but that's probably binary.

@codeboten codeboten merged commit 3ad6ac5 into open-telemetry:master Jun 2, 2020
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Jun 5, 2020
sethmaxwl pushed a commit to sethmaxwl/opentelemetry-python that referenced this pull request Jun 5, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* docs: add ioredis example

* refactor: simplify example

* fix: ioredis example

* fix: tracerRegistry not tracer

* fix: test only with set command

* fix: only use Jaeger as backend

Signed-off-by: Naseem <[email protected]>

Co-authored-by: Daniel Dyla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
instrumentation Related to the instrumentation of third party libraries or frameworks needs reviewers PRs with this label are ready for review and needs people to review to move forward.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add instrumentation for Boto
3 participants