canonical / tempo-k8s-operator

This charmed operator automates the operational procedures of running Grafana Tempo, an open-source tracing backend.
https://charmhub.io/tempo-k8s
Apache License 2.0
3 stars 3 forks source link

charm_tracing charmlib does not correctly wrap staticmethods in instrumented code #120

Closed shayancanonical closed 1 month ago

shayancanonical commented 1 month ago

Bug Description

The charm_tracing charm lib has a fixme comment which manifested and was observed in mysql-router-k8s-operator's unit tests (see logs below for error trace).

To Reproduce

  1. git clone git@github.com:canonical/mysql-router-k8s-operator.git
  2. git checkout feature/tempo_tracing
  3. virtualenv venv
  4. source venv/bin/activate
  5. pip install poetry tox
  6. tox -e unit

Environment

tempo_k8s charm_tracing lib: v1.6

Relevant log output

tests/unit/test_workload.py:188: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _  

args = ('# File automatically generated during MySQL Router bootstrap\n[DEFAULT]\nname=system\nkeyring_path=/var/lib/mysqlrou...e\n\n[rest_routing]\nrequire_realm=default_auth_realm\n\n[rest_metadata_cache]\nrequire_realm=default_auth_realm\n\n',)                
kwargs = {}, name = 'AuthenticatedWorkload._parse_username_from_config'                                  

    @functools.wraps(callable)
    def wrapped_function(*args, **kwargs):  # type: ignore                                               
        name = getattr(callable, "__qualname__", getattr(callable, "__name__", str(callable)))           
        with _span(f"{'(static) ' if static else ''}{qualifier} call: {name}"):  # type: ignore          
            if static:                              
                # fixme: do we or don't we need [1:]?                                                    
                #  The _trace_callable decorator doesn't always play nice with @staticmethods.           
                #  Sometimes it will receive 'self', sometimes it won't.                                 
                # return callable(*args, **kwargs)  # type: ignore                                       
>               return callable(*args[1:], **kwargs)  # type: ignore                                     
E               TypeError: AuthenticatedWorkload._parse_username_from_config() missing 1 required positional argument: 'config_file_text'

Additional context

static method signature:

@staticmethod
def _parse_username_from_config(config_file_text: str) -> str:
PietroPasotti commented 1 month ago

Yeah that one has been bugging me for ages, never managed to figure out what's going on. Will see if I can write a consistent reproducer for both failure modes, thanks for the reference

PietroPasotti commented 1 month ago

Huh, found out what the issue is. image

Calling a static method on an instance passes, calling it on the type, fails. Working on a fix...