Skip to content

Commit a3deaea

Browse files
authored
fix(internal): introduce better error logging when we cannot pack a span [backport #5797 to 1.13] (#5805)
Backport of #5797 to 1.13 If we end up with a meta value that is not a string it can raise a cryptic error that doesn't help deubgging. This change introduces a new error message that includes the `repr(span)` that caused the issue. This change will *not* include the meta for the span, but having the span name should help narrow things down. ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines) are followed. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] PR description includes explicit acknowledgement/acceptance of the performance implications of this PR as reported in the benchmarks PR comment. ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment.
1 parent 5ba0d09 commit a3deaea

File tree

3 files changed

+21
-3
lines changed

3 files changed

+21
-3
lines changed

ddtrace/internal/_encoding.pyx

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -474,8 +474,14 @@ cdef class MsgpackEncoderBase(BufferedEncoder):
474474
dd_origin = self.get_dd_origin_ref(trace[0].context.dd_origin)
475475

476476
for span in trace:
477-
ret = self.pack_span(span, dd_origin)
478-
if ret != 0: raise RuntimeError("Couldn't pack span")
477+
try:
478+
ret = self.pack_span(span, dd_origin)
479+
except Exception as e:
480+
raise RuntimeError("failed to pack span: {!r}. Exception: {}".format(span, e))
481+
482+
# No exception was raised, but we got an error code from msgpack
483+
if ret != 0:
484+
raise RuntimeError("couldn't pack span: {!r}".format(span))
479485

480486
return ret
481487

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
fixes:
2+
- |
3+
tracing: Fixes a cryptic encoding exception message when a span tag is not a string.

tests/tracer/test_encoders.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
# -*- coding: utf-8 -*-
2+
import contextlib
23
import json
34
import random
45
import string
@@ -650,6 +651,11 @@ def test_list_string_table():
650651
assert list(t) == ["", "foobar", "foobaz"]
651652

652653

654+
@contextlib.contextmanager
655+
def _value():
656+
yield "value"
657+
658+
653659
@pytest.mark.parametrize(
654660
"data",
655661
[
@@ -663,6 +669,8 @@ def test_list_string_table():
663669
{"duration_ns": "duration_time"},
664670
{"span_type": 100},
665671
{"_meta": {"num": 100}},
672+
# Validating behavior with a context manager is a customer regression
673+
{"_meta": {"key": _value()}},
666674
{"_metrics": {"key": "value"}},
667675
],
668676
)
@@ -674,9 +682,10 @@ def test_encoding_invalid_data(data):
674682
setattr(span, key, value)
675683

676684
trace = [span]
677-
with pytest.raises(TypeError):
685+
with pytest.raises(RuntimeError) as e:
678686
encoder.put(trace)
679687

688+
assert e.match(r"failed to pack span: <Span\(id="), e
680689
assert encoder.encode() is None
681690

682691

0 commit comments

Comments
 (0)