Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions packages/service-library/src/servicelib/fastapi/tracing.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,16 @@

"""

import importlib
import importlib.machinery
Copy link
Member

Choose a reason for hiding this comment

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

very cool name...

import inspect
import logging
import sys
from functools import wraps
from importlib.abc import Loader, MetaPathFinder
from importlib.machinery import ModuleSpec
from types import ModuleType
from typing import Callable, Sequence

from fastapi import FastAPI
from httpx import AsyncClient, Client
Expand Down Expand Up @@ -127,3 +136,66 @@ def setup_tracing(

def setup_httpx_client_tracing(client: AsyncClient | Client):
HTTPXClientInstrumentor.instrument_client(client)


def _opentelemetry_function_span(func: Callable):
"""Decorator that wraps a function call in an OpenTelemetry span."""
tracer = trace.get_tracer(__name__)

@wraps(func)
def wrapper(*args, **kwargs):
with tracer.start_as_current_span(f"{func.__module__}.{func.__name__}"):
return func(*args, **kwargs)

@wraps(func)
async def async_wrapper(*args, **kwargs):
with tracer.start_as_current_span(f"{func.__module__}.{func.__name__}"):
return await func(*args, **kwargs)

if inspect.iscoroutinefunction(func):
return async_wrapper
else:
return wrapper


def _opentelemetry_method_span(cls):
Copy link
Member

Choose a reason for hiding this comment

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

here I have some questions:

  • how heavy is using this? If I get it right, using this wraps every single function with a network call to the telemetry collector right? That means any call is network-limited is that correct?
  • if yes, then what happens if the collector is broken or the network is shaky?
  • does this break the function?
  • if there are network issues, does it retry? how many times? what is the timeout?
  • if you telemetrize a ping function for example, that might be quite bad no? these function typically should run with no "brakes", can you exclude functions?

for name, value in cls.__dict__.items():
if callable(value) and not name.startswith("_"):
Copy link
Member

Choose a reason for hiding this comment

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

what about properties, or (in general) descriptors , do you decorate them?

setattr(cls, name, _opentelemetry_function_span(value))
Copy link
Member

Choose a reason for hiding this comment

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

so here value is a callable and you basically decorate the member functions.
MINOR

setattr(cls, name, _opentelemetry_function_span(func=value))

return cls


class _AddTracingSpansLoader(Loader):
def __init__(self, loader: Loader):
self.loader = loader

def exec_module(self, module: ModuleType):
self.loader.exec_module(module)
for name, func in inspect.getmembers(module, inspect.isfunction):
if name in module.__dict__:
setattr(module, name, _opentelemetry_function_span(func))
for name, cls in inspect.getmembers(module, inspect.isclass):
if name in module.__dict__ and cls.__module__ == module.__name__:
setattr(module, name, _opentelemetry_method_span(cls))


class _AddTracingSpansFinder(MetaPathFinder):
Copy link
Member

Choose a reason for hiding this comment

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

MetaPathFinder ... ?? this sounds like a wonder name for a spaceship 🚀 :-D

def find_spec(
self,
fullname: str,
path: Sequence[str] | None,
target: ModuleType | None = None,
) -> ModuleSpec | None:
if fullname.startswith("simcore_service"):
spec = importlib.machinery.PathFinder.find_spec(
fullname=fullname, path=path
)
if spec and spec.loader:
spec.loader = _AddTracingSpansLoader(spec.loader)
return spec

return None


def setup_tracing_spans_for_simcore_service_functions():
Copy link
Member

Choose a reason for hiding this comment

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

why dont you pass the name of the module instead of relying on things that were imported?

If i undestand it right, yoy want to decorate all members and functions of a given module so the steps would be

  1. import all modules in a package
  2. inspect all members and functions
  3. apply decorators above

for 1 and 2 see what we do with walk_model_examples_in_package. I guess is similar to the MetaPathFinder but higher level since uses built-in pkgutil.walk_packages

sys.meta_path.insert(0, _AddTracingSpansFinder())
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@
from models_library.basic_types import BootModeEnum
from packaging.version import Version
from servicelib.fastapi.profiler_middleware import ProfilerMiddleware
from servicelib.fastapi.tracing import setup_tracing
from servicelib.fastapi.tracing import (
setup_tracing,
setup_tracing_spans_for_simcore_service_functions,
)
from servicelib.logging_utils import config_all_loggers
from simcore_service_api_server import exceptions
Copy link
Member

Choose a reason for hiding this comment

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

we use relative imports ... from .. import exceptions
Actually, perhaps you can do some cleanup? :-)
image

Exceptionally main might need absolute import, otherwise we should use relavite imports everywhere.


setup_tracing_spans_for_simcore_service_functions()
Copy link
Member

Choose a reason for hiding this comment

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

I really do not like that the setup has to be called depending on the imports
if it is experimental, why not to use DEV_FEATURE to enable/disable it?
I assume you will be on top of it right?

Copy link
Member

Choose a reason for hiding this comment

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

should this not be called in your import directly? I have the feeling this is very fragile as this will for sure move with some of our tools.

also, this instruments everything? I guess my questions above then remain more than ever.

# isort: split

from .. import exceptions
from .._meta import API_VERSION, API_VTAG, APP_NAME
from ..api.root import create_router
from ..api.routes.health import router as health_router
Expand Down
Loading