From 8f75869b6a6285a9d4d73395ff965f529c091f32 Mon Sep 17 00:00:00 2001
From: "James E. Blair" <jim@acmegating.com>
Date: Tue, 30 Jul 2024 11:05:42 -0700
Subject: [PATCH] Fix encoding of null attributes in tracing

OTLP can't handle NoneType attributes, which is what we have in
the case of a change with no oldrev or newrev.  To correct this,
avoid adding change attributes to a tracing span if they are None.

Add tracing tests for post and timer-based jobs to exercise this
condition.

Change-Id: I1590a3af9c1bf029377b375f9a6eff777a9f0770
---
 tests/unit/test_tracing.py | 49 ++++++++++++++++++++++++++++++++++----
 zuul/manager/__init__.py   |  3 ++-
 2 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/tests/unit/test_tracing.py b/tests/unit/test_tracing.py
index 8d5d080944..8da43915b3 100644
--- a/tests/unit/test_tracing.py
+++ b/tests/unit/test_tracing.py
@@ -45,6 +45,12 @@ class TestTracing(ZuulTestCase):
             if len(test_requests) == len(span_names):
                 return test_requests
 
+    def getSpan(self, name):
+        for req in self.otlp.requests:
+            span = req.resource_spans[0].scope_spans[0].spans[0]
+            if span.name == name:
+                return span
+
     def test_tracing_api(self):
         tracer = trace.get_tracer("zuul")
 
@@ -255,8 +261,41 @@ class TestTracing(ZuulTestCase):
         self.assertTrue(item_attrs['ref_patchset'] == ["1"])
         self.assertTrue('zuul_event_id' in item_attrs)
 
-    def getSpan(self, name):
-        for req in self.otlp.requests:
-            span = req.resource_spans[0].scope_spans[0].spans[0]
-            if span.name == name:
-                return span
+    def test_post(self):
+        "Test that post jobs run"
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        A.setMerged()
+        self.fake_gerrit.addEvent(A.getRefUpdatedEvent())
+        self.waitUntilSettled()
+
+        self.assertHistory([
+            dict(name='project-post', result='SUCCESS',
+                 ref='refs/heads/master'),
+        ], ordered=False)
+
+        job_names = [x.name for x in self.history]
+        self.assertEqual(len(self.history), 1)
+        self.assertIn('project-post', job_names)
+
+    def test_timer(self):
+        self.executor_server.hold_jobs_in_build = True
+        self.commitConfigUpdate('common-config', 'layouts/timer.yaml')
+        self.scheds.execute(lambda app: app.sched.reconfigure(app.config))
+        self.waitUntilSettled()
+
+        for _ in iterate_timeout(30, 'Wait for a build on hold'):
+            if len(self.builds) > 0:
+                break
+        self.waitUntilSettled()
+
+        self.commitConfigUpdate('common-config',
+                                'layouts/no-timer.yaml')
+        self.scheds.execute(lambda app: app.sched.reconfigure(app.config))
+        self.waitUntilSettled()
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+        self.assertHistory([
+            dict(name='project-bitrot', result='SUCCESS',
+                 ref='refs/heads/master'),
+        ], ordered=False)
diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py
index 53b3c595b0..01535af4df 100644
--- a/zuul/manager/__init__.py
+++ b/zuul/manager/__init__.py
@@ -1004,7 +1004,8 @@ class PipelineManager(metaclass=ABCMeta):
         }
         for change in item.changes:
             for k, v in change.getSafeAttributes().toDict().items():
-                span_attrs.setdefault(f'ref_{k}', []).append(v)
+                if v is not None:
+                    span_attrs.setdefault(f'ref_{k}', []).append(v)
         tracing.endSavedSpan(item.current_build_set.span_info)
         tracing.endSavedSpan(item.span_info,
                              attributes=span_attrs)