Skip to content

Commit 16a6c92

Browse files
authored
fix(internal): introduce better error logging when we cannot pack a span [backport #5797 to 1.12] (#5806)
Backport of #5797 to 1.12 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 e54044e commit 16a6c92

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
@@ -648,6 +649,11 @@ def test_list_string_table():
648649
assert list(t) == ["", "foobar", "foobaz"]
649650

650651

652+
@contextlib.contextmanager
653+
def _value():
654+
yield "value"
655+
656+
651657
@pytest.mark.parametrize(
652658
"data",
653659
[
@@ -661,6 +667,8 @@ def test_list_string_table():
661667
{"duration_ns": "duration_time"},
662668
{"span_type": 100},
663669
{"_meta": {"num": 100}},
670+
# Validating behavior with a context manager is a customer regression
671+
{"_meta": {"key": _value()}},
664672
{"_metrics": {"key": "value"}},
665673
],
666674
)
@@ -672,9 +680,10 @@ def test_encoding_invalid_data(data):
672680
setattr(span, key, value)
673681

674682
trace = [span]
675-
with pytest.raises(TypeError):
683+
with pytest.raises(RuntimeError) as e:
676684
encoder.put(trace)
677685

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

680689

0 commit comments

Comments
 (0)