fix(traces): a before_span_send entry that is not callable turns tracing off

Warning and exporting without the entry was the opposite of what a
raising hook does, and the opposite of the fail-closed rationale in the
client. The resolver now raises, so the client reports the error and
leaves tracing off rather than exporting spans the entry was meant to
redact.

Author: turnipdabeets

posthog/test/tracing/test_config.py

@@ -256,15 +256,14 @@ def hook(span):
         assert resolved.before_span_send == (hook,)
         assert not caplog.records
 
-    def test_ignores_and_warns_about_entries_that_are_not_callable(self, caplog):
-        caplog.set_level("WARNING", logger="posthog")
-
+    def test_rejects_an_entry_that_is_not_callable_rather_than_exporting_unhooked(
+        self,
+    ):
         def hook(span):
             return span
 
-        resolved = resolve_traces_config({"before_span_send": ["scrub", hook]})
-        assert resolved.before_span_send == (hook,)
-        assert any("1 of 2" in r.getMessage() for r in caplog.records)
+        with pytest.raises(ValueError, match="not callable"):
+            resolve_traces_config({"before_span_send": ["scrub", hook]})
 
     def test_a_hook_whose_truthiness_raises_is_still_resolved(self):
         class Hook:

posthog/test/tracing/test_pipeline.py

@@ -761,13 +761,6 @@ def third(span):
         assert calls == ["first", "second"]
         assert queued(pipeline) == []
 
-    def test_ignores_non_callable_entries_and_exports_unhooked(self, caplog):
-        caplog.set_level("WARNING", logger="posthog")
-        pipeline, _, _ = make(before_span_send=["not a hook", None])
-        pipeline.start_span("a").end()
-        assert len(queued(pipeline)) == 1
-        assert any("not callable" in r.getMessage() for r in caplog.records)
-
     def test_a_hook_can_remove_the_stacktrace(self):
         def strip_stacks(span):
             for event in span["events"]:

posthog/tracing/_config.py

@@ -123,26 +123,24 @@ def _usable_resource_attributes(value: Any) -> Dict[str, Any]:
 
 
 def _resolve_before_span_send(value: Any) -> Tuple[Callable, ...]:
-    """Keep only the callable hooks, in order.
+    """The hooks, in order. Raises for an entry that is not callable.
 
-    Anything else is dropped rather than called: a hook that raises drops every
-    span, which would leave tracing silently off.
+    The hook is the scrubbing point, so a broken entry turns tracing off
+    rather than exporting spans the entry was meant to redact. The client
+    reports the error and leaves tracing off for the life of the client.
     """
     if value is None:
         return ()
     supplied = list(value) if isinstance(value, (list, tuple)) else [value]
     # `[enabled and scrub]` yields None or False: no hook, rather than a broken one.
     supplied = [hook for hook in supplied if hook is not None and hook is not False]
-    hooks = tuple(hook for hook in supplied if callable(hook))
-    if len(hooks) != len(supplied):
-        log.warning(
-            "Ignoring %s of %s traces before_span_send entries that are not callable. "
-            "Spans export without them, so whatever they were redacting is not "
-            "redacted.",
-            len(supplied) - len(hooks),
-            len(supplied),
-        )
-    return hooks
+    for hook in supplied:
+        if not callable(hook):
+            raise ValueError(
+                "traces before_span_send entry {!r} is not callable; tracing is off "
+                "rather than exporting spans it was meant to redact".format(hook)
+            )
+    return tuple(supplied)
 
 
 def resolve_traces_config(