-
Notifications
You must be signed in to change notification settings - Fork 628
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 a rich console exporter #686
Conversation
Thanks. This looks very nice and useful but is this something that the OpenTelemetry project should host and maintain? This could easily be hosted independently and published as a 3rd party package to pypi.org. Curious what other @open-telemetry/python-approvers @open-telemetry/python-maintainers think about it. |
If we were to host it in this repo, I think it'd be better to add the feature to the existing console exporter and make it configurable to print either format. |
Happy to host + publish this one myself. Can I use the same package name ( If it were added to the console exporter, you'd have to add Rich as a dependency to the console exporter, which would be overkill IMO |
Making it optional dependency with |
I think so. Unfortunately pypi doesn't have namespaces like NPM and I don't think we can enforce |
@owais how does this differ from other exporters/instrumentations in this repo? Trying to understand arguments for and against this being merged here. |
@aabmass I'm not sure either and that is why I'm bringing it up. I don't think there is anything inherently wrong with it being in contrib but at the same time every new package adds additional burden on maintainers and systems (CI) both. It is usually worth it for packages that improve the quality of telemetry this project can provide (instrumentations) but not sure if every dev/debugging utility needs to be hosted in this repo. May be we can encourage the broader OpenTelemetry (Python) ecosystem to grow beyond core and contrib repos. I don't feel strongly either way but felt it was an interesting this to talk about. |
@owais I think this one adds a lot of value for developers (both instrumentation authors and application developers). I'm in favor of adding this, but I agree it's worth a general discussion. I've added this PR to the agenda for next week's SIG. |
@tonybaloney please fix lint issues, add a changelog entry and we can merge 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.
A few comments, otherwise LGTM
exporter/opentelemetry-exporter-richconsole/src/opentelemetry/exporter/richconsole/__init__.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-richconsole/src/opentelemetry/exporter/richconsole/__init__.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-richconsole/src/opentelemetry/exporter/richconsole/__init__.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Aaron Abbott <aaronabbott@google.com>
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. Added couple of comments.
exporter/opentelemetry-exporter-richconsole/src/opentelemetry/exporter/richconsole/__init__.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-richconsole/src/opentelemetry/exporter/richconsole/__init__.py
Outdated
Show resolved
Hide resolved
Whats the correct way to run the linter to get the rest of this to pass? I tried running black but it reformats everything, tried |
@tonybaloney make sure your installed black and isort versions are the same as pinned in dev-requirements.txt, then run |
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.
Just a couple of comments, this is a good debugging tool!
exporter/opentelemetry-exporter-richconsole/src/opentelemetry/exporter/richconsole/__init__.py
Outdated
Show resolved
Hide resolved
@codeboten done and done! |
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!
@aabmass please approve if your comments have been addressed |
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.
🚢
Description
This is an alternative to the Console Exporter, that uses the Rich Console library to print colorised trees from a batch span processor. Span attributes are shown as nodes in the tree and spans are children of their related parent span/trace.
This is easier to read than the console exporter and see the relationships between traces.
Useful for debugging.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.
Example output:
Colorised: