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

[PLACEHOLDER] Otel-like tracing #1346

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Conversation

sfc-gh-pmardziel
Copy link
Contributor

@sfc-gh-pmardziel sfc-gh-pmardziel commented Aug 16, 2024

Placeholder. Splitting off into smaller pieces:

Description

  • Adds trulens.core.preview to manage preview/experimental feature flags.

  • Enables otel-like tracing of spans if the appropriate flag is set.

  • *** Refactored a lot of imports in terms of module imports to avoid circular import issue. ***

Other details good to know for developers

As part of this PR, I had to convert a lot of imports of values to imports of modules instead to help alleviate circular import issues. That is, instead of

from trulens.core.tru import Tru

Were converted to

from trulens.core import tru as mod_tru

Following usage of Tru was in terms of mod_tru.Tru. This was already called out in our style guide (https://google.github.io/styleguide/pyguide.html#22-imports) but needs to be taken up more now as packaging/moduling is becoming more complex.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • New Tests
  • This change includes re-generated golden test results
  • This change requires a documentation update

@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Aug 16, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@sfc-gh-pmardziel sfc-gh-pmardziel marked this pull request as draft August 16, 2024 01:04
@sfc-gh-pmardziel sfc-gh-pmardziel changed the title Otel-like tracing flag [DRAFT] Otel-like tracing flag Aug 16, 2024
@@ -0,0 +1,146 @@
{
Copy link
Contributor

@sfc-gh-jreini sfc-gh-jreini Aug 19, 2024

Choose a reason for hiding this comment

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

Line #12.    # Install zipkin python package:

Possible to show this with the standard opentelemetry collector instead of zipkin? https://opentelemetry.io/docs/collector/quick-start/


Reply via ReviewNB

Copy link
Contributor Author

@sfc-gh-pmardziel sfc-gh-pmardziel Aug 19, 2024

Choose a reason for hiding this comment

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

You mean the toy collector that merely outputs the spans to stdout which they show being sent to a text file in that example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, maybe in addition? Want to show more clearly what the spans look like.

Unrelatedly, how easy is it for someone to choose a different exporter? Should we show this for 2-3 of the most popular ones?

examples/experimental/export_example.ipynb Show resolved Hide resolved
@@ -0,0 +1,146 @@
{
Copy link
Contributor

@sfc-gh-jreini sfc-gh-jreini Aug 19, 2024

Choose a reason for hiding this comment

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

Line #1.    # Nested context managers.

what is the point of showing this nested context manager? what functionality is displayed?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was for testing but I see that if this is a generic export example, a more interesting/more relevant recording is warranted. Will work on that.

@@ -0,0 +1,146 @@
{
Copy link
Contributor

@sfc-gh-jreini sfc-gh-jreini Aug 19, 2024

Choose a reason for hiding this comment

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

what does this output?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spans that were recorded during the invocations above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to keep output for this cell to show what the spans look like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to figure out how to keep outputs in notebooks as the recent packaging change clears all notebook outputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For docs at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, looks like the clearing is done in the pre-commit hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sfc-gh-chu : Is there a way to prevent the notebook output to be cleared during pre-commit or doc generation?

@sfc-gh-pmardziel sfc-gh-pmardziel changed the title [DRAFT] Otel-like tracing flag Otel-like tracing Aug 19, 2024
@sfc-gh-pmardziel sfc-gh-pmardziel marked this pull request as ready for review August 19, 2024 20:02
@dosubot dosubot bot added the documentation Improvements or additions to documentation label Aug 19, 2024
@sfc-gh-jreini
Copy link
Contributor

Is the addition of span types, where we can realize the benefit of easier selectors, planned for a future change?

@sfc-gh-pmardziel sfc-gh-pmardziel changed the title Otel-like tracing [PLACEHOLDER] Otel-like tracing Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants