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 tracing to Ray #14872

Merged
merged 129 commits into from
May 1, 2021
Merged

Add tracing to Ray #14872

merged 129 commits into from
May 1, 2021

Conversation

kathryn-zhou
Copy link
Contributor

@kathryn-zhou kathryn-zhou commented Mar 23, 2021

Why are these changes needed?

This PR adds a prototype for OpenTelemetry tracing in Python level for Ray tasks and actors.

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@simon-mo
Copy link
Contributor

can you move all functions in the core files, and the dict_propogator, to a new file called tracing.py?

@simon-mo simon-mo self-assigned this Mar 23, 2021
Copy link
Contributor

@simon-mo simon-mo left a comment

Choose a reason for hiding this comment

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

let's add docs. and figure out consistent naming scheme:

  • make_/inject_
  • tracing_* task/actors.

@@ -0,0 +1,15 @@
from typing import Any, cast, Dict

Copy link
Contributor

Choose a reason for hiding this comment

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

inline this to tracing

self._function_name = (
self._function.__module__ + "." + self._function.__name__)
function.__module__ + "." + function.__name__)
print(f"function name is {self._function_name}")
Copy link
Contributor

Choose a reason for hiding this comment

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

pop this line

@@ -105,6 +110,33 @@ def _remote_proxy(*args, **kwargs):

self.remote = _remote_proxy

@staticmethod
def _tracing_wrap_function(function):
Copy link
Contributor

Choose a reason for hiding this comment

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

move this to a pure function in tracing.py

Copy link
Contributor

Choose a reason for hiding this comment

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

naming: inject_tracing_into_function(function)

python/ray/util/tracing.py Outdated Show resolved Hide resolved


@contextmanager
def use_context(parent_context: Context, ) -> Generator[None, None, None]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def use_context(parent_context: Context, ) -> Generator[None, None, None]:
def use_context(parent_context: Context) -> Generator[None, None, None]:

python/ray/actor.py Show resolved Hide resolved
@@ -131,6 +131,7 @@ def remote(self, *args, **kwargs):

return FuncWrapper()

@actor_method_tracing_local
Copy link
Contributor

Choose a reason for hiding this comment

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

naming: tracing_actor_invocation

@@ -169,6 +201,7 @@ def remote(self, *args, **kwargs):

return FuncWrapper()

@_propagate_trace
Copy link
Contributor

Choose a reason for hiding this comment

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

tracing_task_invocation

from contextlib import contextmanager
import inspect
import os
import wrapt
Copy link
Contributor

Choose a reason for hiding this comment

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

let's try to remove wrapt as a dependency

python/ray/util/tracing.py Outdated Show resolved Hide resolved
@rkooo567 rkooo567 self-assigned this Mar 24, 2021
# Skip tracing for staticmethod or classmethod, because these method
# might not be called directly by remote calls. Additionally, they are
# tricky to get wrapped and unwrapped.
if is_static_method(_cls, name) or is_class_method(method):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if is_static_method(_cls, name) or is_class_method(method):
if is_static_method(_cls, name) or is_class_method(method) or not is_tracing_enabled():

and remove it from span_wrapper and async_span_wrapper

@simon-mo simon-mo merged commit 38b64e4 into ray-project:master May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants