Make open-telemetry a soft dependency [RHELDST-27627] #196

amcmahon-rh · 2024-11-07T13:02:49Z

Several client tools (e.g. rhsm-tools) make use of various parts of pubtools for interaction with pulp. This has caused issues with rcm-dev, as we have specifically avoided making open-telemetry packages available for CLI clients.

This change introduces a new TracingWrapper class that acts as a pass through for when opentelemetry isn't installed.

rohanpm · 2024-11-07T21:23:11Z

src/pubtools/_impl/tracing.py

+        if OPENTELEMETRY_AVAILABLE:
+            TRACE_WRAPPER = ServerTracingWrapper()
+        else:
+            log.warning(


I would prefer not to WARN here, the problem is that we don't really have control over where get_trace_wrapper is called, and how many times.

This could easily result in several duplicate warnings produced every time certain tools are run, confusing the users. Note, if a tool fails for any reason, and it output some warnings prior to the failure, there's usually going to be some users who assume the warned-about problem caused the failure.

Can we perhaps do one of these?

make it DEBUG instead of WARN - that means it's still possible to tell whether this code path is taken by increasing the log level.

or keep it as WARN but only if OTEL_TRACING environment variable has been set to true (the same condition used to initialize _enabled_trace). Because this would indicate that someone is explicitly asking for OTEL tracing in an environment where it's not available.

I've reworked this change to just disable the tracing if OTEL isn't available, this is much more straight forward. I've changed the log to debug. I think it's important to have some logging here, as we're silencing an exception.

rohanpm · 2024-11-07T21:26:15Z

src/pubtools/_impl/tracing.py

+                "Open-telemetry package is not available, falling "
+                "back to pass through tracer."
+            )
+            CLITracingWrapper()


Shouldn't this be assigned to TRACE_WRAPPER ?

src/pubtools/_impl/tracing.py

rohanpm · 2024-11-07T21:49:44Z

src/pubtools/_impl/tracing.py

+        def _instrument_func(func):
+            @functools.wraps(func)
+            def wrap(_):
+                pass


This seems to be broken in two ways:

func is never called, so this decorator replaces every decorated function with a replacement which does nothing.

wrap accepts only a single positional argument. This means @instrument_func will break any function with a signature other than "single positional argument". The general recipe for a generic decorator is to accept any number of positional and named arguments.

These are severe bugs to the point of not working at all, so if there aren't any tests which caught this, there should be some.

That being said, if you follow my suggestion in the other thread of not introducing a separate class for this, we'd be reusing the existing instrument_func implementation which is already tested.

rohanpm · 2024-11-07T21:51:36Z

src/pubtools/_impl/tracing.py

+
+    def force_flush(self):
+        """Flush trace data into OTEL collectors"""
+        log.warning(


I don't think this warrants a warning, I think it can just be understood that force_flush does nothing if the feature is disabled.

The nature of the function is already that it can potentially do nothing (as there is not necessarily any data in need of flushing).

rohanpm · 2024-11-07T21:55:42Z

tests/tracing/test_instrument_tracing.py

+
+
+def test_client_tracing_fallback(monkeypatch):
+    def mock_import(name, *args, **kwargs):


I don't get how this test could work:

pubtools._impl.tracing has already been imported and OPENTELEMETRY_AVAILABLE initialized at line 7 of this file. This is long before test_client_tracing_fallback has been invoked, and before monkeypatch.setattr is called.

Have you confirmed that this code is actually reached?

rohanpm

One more thing which might be missing from this:

requirements.txt still unconditionally depends on the otel libraries.

I know the ultimate goal is to fix problems with some RPMs, so one easy option is to just patch out those dependencies during RPM build, knowing that the source code is now able to cope with it.

However you might also consider grouping those as "extra" dependencies, which I believe would look like this:

diff --git a/requirements.txt b/requirements.txt
index a38c65c..c0ec2d3 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,4 +1,2 @@
 pluggy
 setuptools
-opentelemetry-api
-opentelemetry-sdk
diff --git a/setup.py b/setup.py
index 100b504..cfc26d3 100644
--- a/setup.py
+++ b/setup.py
@@ -39,6 +39,9 @@ setup(
         "Topic :: Software Development :: Libraries :: Python Modules",
     ],
     install_requires=get_requirements(),
+    extras_require={
+        "tracing": ["opentelemetry-api", "opentelemetry-sdk"],
+    },
     python_requires=">=3.6",
     project_urls={
         "Documentation": "https://release-engineering.github.io/pubtools/",

That would make the otel libraries not required by default,
while a service like Pub could declare a dependency on
pubtools[tracing] rather than just pubtools in order to pull them in.

rohanpm · 2024-11-11T21:31:16Z

src/pubtools/_impl/tracing.py

+        if (
+            os.getenv("OTEL_TRACING", "").lower() == "true"
+        ) and not OPENTELEMETRY_AVAILABLE:
+            log.debug(


Ideally there should be a test which reaches this line.

You could probably monkeypatch.setattr(tracing, "OPENTELEMETRY_AVAILABLE", False) from within a test to force it to behave as though otel libraries are not available even when they are.

rohanpm · 2024-11-11T21:32:37Z

src/pubtools/_impl/tracing.py

+except ImportError:
+    # Clients aren't expected to have open-telemetry. This flag will be used in
+    # TracingWrapper to provide pass through functions.
+    OPENTELEMETRY_AVAILABLE = False


This could have a pragma: no cover added to pass the coverage check, given that it's probably not worth maintaining two separate test environments with otel present vs missing.

rohanpm · 2024-11-12T22:57:58Z

Could you please rebase it? The dependency files were bumped since your last commit.

Several client tools (e.g. rhsm-tools) make use of various parts of pubtools for interaction with pulp. This has caused issues with rcm-dev, as we have specifically avoided making open-telemetry packages available for CLI clients. This change introduces a new TracingWrapper class that acts as a pass through for when opentelemetry isn't installed.

for more information, see https://pre-commit.ci

amcmahon-rh requested review from rohanpm, rbikar and querti as code owners November 7, 2024 13:02

rohanpm requested changes Nov 7, 2024

View reviewed changes

amcmahon-rh force-pushed the otel-soft-dep branch 2 times, most recently from 2c6a40a to 03287c5 Compare November 11, 2024 12:15

rohanpm reviewed Nov 11, 2024

View reviewed changes

amcmahon-rh force-pushed the otel-soft-dep branch from 9372879 to 765a0e8 Compare November 12, 2024 16:30

rohanpm approved these changes Nov 12, 2024

View reviewed changes

amcmahon-rh force-pushed the otel-soft-dep branch from 765a0e8 to 4b08f3b Compare November 13, 2024 12:18

amcmahon-rh force-pushed the otel-soft-dep branch from 1035c09 to ab3895a Compare November 13, 2024 12:42

[pre-commit.ci] auto fixes from pre-commit.com hooks

8928f6f

for more information, see https://pre-commit.ci

amcmahon-rh merged commit b66561b into release-engineering:main Nov 13, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make open-telemetry a soft dependency [RHELDST-27627] #196

Make open-telemetry a soft dependency [RHELDST-27627] #196

amcmahon-rh commented Nov 7, 2024

rohanpm Nov 7, 2024

amcmahon-rh Nov 11, 2024

rohanpm Nov 7, 2024

rohanpm Nov 7, 2024

rohanpm Nov 7, 2024

rohanpm Nov 7, 2024

rohanpm left a comment

rohanpm Nov 11, 2024

rohanpm Nov 11, 2024

rohanpm commented Nov 12, 2024



		def test_client_tracing_fallback(monkeypatch):
		def mock_import(name, args, *kwargs):

Make open-telemetry a soft dependency [RHELDST-27627] #196

Make open-telemetry a soft dependency [RHELDST-27627] #196

Conversation

amcmahon-rh commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohanpm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohanpm commented Nov 12, 2024