Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for array attributes to Zipkin exporter #1285

Merged
merged 20 commits into from
Dec 16, 2020
Merged

Adding support for array attributes to Zipkin exporter #1285

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
c10311a
Adding support for array attributes to Zipkin exporter (#1110)
robwknox Oct 26, 2020
83a868c
lint changes (#1110)
robwknox Oct 26, 2020
8d3f204
Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs
robwknox Oct 26, 2020
779b090
CHANGELOG entry (#1110)
robwknox Oct 27, 2020
b31c881
Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs
robwknox Nov 13, 2020
1033a76
changelog
robwknox Nov 13, 2020
1d3a63f
lint
robwknox Nov 13, 2020
95dfae4
Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs
lzchen Nov 19, 2020
0082859
Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs
lzchen Nov 19, 2020
d82fb30
Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs
robwknox Nov 23, 2020
a2f1c8d
Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs
lzchen Nov 24, 2020
75edd7d
Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs
Nov 30, 2020
014f5e4
review refactor
robwknox Dec 2, 2020
2a5c4b3
review refactor
robwknox Dec 3, 2020
85ef28c
Merge branch '1110_zipkin_exporter_support_array_attrs' of https://gi…
robwknox Dec 3, 2020
51d8024
black
robwknox Dec 3, 2020
a226e19
black
robwknox Dec 3, 2020
907ead5
simplify tag value formatting, plus gracefully skip invalid sequence …
robwknox Dec 3, 2020
32d68e8
Merge remote-tracking branch 'origin/master' into 1110_zipkin_exporte…
Dec 14, 2020
7fc3ac9
Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs
Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions exporter/opentelemetry-exporter-zipkin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
Released 2020-11-25

- Support for v2 api protobuf format ([#1318](https://github.com/open-telemetry/opentelemetry-python/pull/1318))
- Add support for array attributes in Span and Resource exports ([#1285](https://github.com/open-telemetry/opentelemetry-python/pull/1285))

## Version 0.14b0

Expand Down
46 changes: 46 additions & 0 deletions exporter/opentelemetry-exporter-zipkin/src/opentelemetry/exporter/zipkin/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,13 @@ def _extract_tags_from_dict(self, tags_dict):
value = str(attribute_value)
elif isinstance(attribute_value, str):
value = attribute_value
elif isinstance(attribute_value, Sequence):
value = self._extract_tag_value_string_from_sequence(
attribute_value
)
if not value:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not value:
if value is None:

logger.warning("Could not serialize tag %s", attribute_key)
continue
else:
logger.warning("Could not serialize tag %s", attribute_key)
continue
Expand All @@ -364,6 +371,45 @@ def _extract_tags_from_dict(self, tags_dict):
tags[attribute_key] = value
return tags

def _extract_tag_value_string_from_sequence(self, sequence):
if not sequence:
return None

if self.max_tag_value_length == 1:
return None
robwknox marked this conversation as resolved.
Show resolved Hide resolved

tag_value_elements = []
running_string_length = (
2 # accounts for array brackets in output string
)
defined_max_tag_value_length = self.max_tag_value_length > 0

if isinstance(sequence, (list, tuple, range)):
robwknox marked this conversation as resolved.
Show resolved Hide resolved
for element in sequence:
if isinstance(element, (int, bool, float)):
tag_value_element = str(element)
elif isinstance(element, str):
tag_value_element = element
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit L354 to 357 repeated here, could be refactored. One complication is sequence attributes are allowed to have None.

Could also combine the two conditions

Suggested change
if isinstance(element, (int, bool, float)):
tag_value_element = str(element)
elif isinstance(element, str):
tag_value_element = element
if isinstance(element, (int, bool, float, str)):
tag_value_element = str(element)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified the formatting as recommended but didn't refactor the duplication as it's small.

I also changed the logic to silently skip invalid sequence elements versus completely failing so that we're passing along as much data s possible.

else:
return None

if defined_max_tag_value_length:
running_string_length += (
len(tag_value_element) + 2
) # +2 accounts for surrounding quotes
if tag_value_elements:
running_string_length += (
2 # accounts for ', ' connector
)
if running_string_length > self.max_tag_value_length:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is max_tag_value_length supposed to be encoded byte length or string length?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're outputting as ASCII with json.dumps() so one and the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure actually so I checked the python docs:

If ensure_ascii is true (the default), the output is guaranteed to have all incoming non-ASCII characters escaped.

So I think if the escape sequences are long, you could get longer length. I think this is OK tho 😄 Maybe just update max_tag_value_length docstring to say its string length if not specified?

break

tag_value_elements.append(tag_value_element)
else:
return None

return json.dumps(tag_value_elements)

def _extract_tags_from_span(self, span: Span):
tags = self._extract_tags_from_dict(getattr(span, "attributes", None))
if span.resource:
Expand Down
202 changes: 196 additions & 6 deletions exporter/opentelemetry-exporter-zipkin/tests/test_zipkin_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,8 +425,23 @@ def test_export_json_max_tag_length(self):
span.start()
span.resource = Resource({})
# added here to preserve order
span.set_attribute("k1", "v" * 500)
span.set_attribute("k2", "v" * 50)
span.set_attribute("string1", "v" * 500)
span.set_attribute("string2", "v" * 50)
span.set_attribute("list1", ["a"] * 25)
span.set_attribute("list2", ["a"] * 10)
span.set_attribute("list3", [2] * 25)
span.set_attribute("list4", [2] * 10)
span.set_attribute("list5", [True] * 25)
span.set_attribute("list6", [True] * 10)
span.set_attribute("tuple1", ("a",) * 25)
span.set_attribute("tuple2", ("a",) * 10)
span.set_attribute("tuple3", (2,) * 25)
span.set_attribute("tuple4", (2,) * 10)
span.set_attribute("tuple5", (True,) * 25)
span.set_attribute("tuple6", (True,) * 10)
span.set_attribute("range1", range(0, 25))
span.set_attribute("range2", range(0, 10))

robwknox marked this conversation as resolved.
Show resolved Hide resolved
span.set_status(Status(StatusCode.ERROR, "Example description"))
span.end()

Expand All @@ -440,8 +455,65 @@ def test_export_json_max_tag_length(self):
_, kwargs = mock_post.call_args # pylint: disable=E0633

tags = json.loads(kwargs["data"])[0]["tags"]
self.assertEqual(len(tags["k1"]), 128)
self.assertEqual(len(tags["k2"]), 50)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid these long testing sequences, may I suggest:

from functools import partial
from json import dumps
...
partial_dumps = partial(dumps, separators=(",", ":"))

self.assertEqual(len(tags["string1"]), 128)
self.assertEqual(len(tags["string2"]), 50)
self.assertEqual(
tags["list1"],
'["a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a"]',
robwknox marked this conversation as resolved.
Show resolved Hide resolved
)
self.assertEqual(
tags["list2"],
'["a", "a", "a", "a", "a", "a", "a", "a", "a", "a"]',
)
self.assertEqual(
tags["list3"],
'["2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2"]',
)
self.assertEqual(
tags["list4"],
'["2", "2", "2", "2", "2", "2", "2", "2", "2", "2"]',
)
self.assertEqual(
tags["list5"],
'["True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True"]',
)
self.assertEqual(
tags["list6"],
'["True", "True", "True", "True", "True", "True", "True", "True", "True", "True"]',
)
self.assertEqual(
tags["tuple1"],
'["a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a", "a"]',
)
self.assertEqual(
tags["tuple2"],
'["a", "a", "a", "a", "a", "a", "a", "a", "a", "a"]',
)
self.assertEqual(
tags["tuple3"],
'["2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2", "2"]',
)
self.assertEqual(
tags["tuple4"],
'["2", "2", "2", "2", "2", "2", "2", "2", "2", "2"]',
)
self.assertEqual(
tags["tuple5"],
'["True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True", "True"]',
)
self.assertEqual(
tags["tuple6"],
'["True", "True", "True", "True", "True", "True", "True", "True", "True", "True"]',
)
self.assertEqual(
tags["range1"],
'["0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", "22"]',
)
self.assertEqual(
tags["range2"],
'["0", "1", "2", "3", "4", "5", "6", "7", "8", "9"]',
)

exporter = ZipkinSpanExporter(service_name, max_tag_value_length=2)
mock_post = MagicMock()
Expand All @@ -452,8 +524,126 @@ def test_export_json_max_tag_length(self):

_, kwargs = mock_post.call_args # pylint: disable=E0633
tags = json.loads(kwargs["data"])[0]["tags"]
self.assertEqual(len(tags["k1"]), 2)
self.assertEqual(len(tags["k2"]), 2)
self.assertEqual(len(tags["string1"]), 2)
self.assertEqual(len(tags["string2"]), 2)
self.assertEqual(tags["list1"], "[]")
self.assertEqual(tags["list2"], "[]")
self.assertEqual(tags["list3"], "[]")
self.assertEqual(tags["list4"], "[]")
self.assertEqual(tags["list5"], "[]")
self.assertEqual(tags["list6"], "[]")
self.assertEqual(tags["tuple1"], "[]")
self.assertEqual(tags["tuple2"], "[]")
self.assertEqual(tags["tuple3"], "[]")
self.assertEqual(tags["tuple4"], "[]")
self.assertEqual(tags["tuple5"], "[]")
self.assertEqual(tags["tuple6"], "[]")
self.assertEqual(tags["range1"], "[]")
self.assertEqual(tags["range2"], "[]")

exporter = ZipkinSpanExporter(service_name, max_tag_value_length=5)
mock_post = MagicMock()
with patch("requests.post", mock_post):
mock_post.return_value = MockResponse(200)
status = exporter.export([span])
self.assertEqual(SpanExportResult.SUCCESS, status)

_, kwargs = mock_post.call_args # pylint: disable=E0633
tags = json.loads(kwargs["data"])[0]["tags"]
self.assertEqual(len(tags["string1"]), 5)
self.assertEqual(len(tags["string2"]), 5)
self.assertEqual(tags["list1"], '["a"]')
self.assertEqual(tags["list2"], '["a"]')
self.assertEqual(tags["list3"], '["2"]')
self.assertEqual(tags["list4"], '["2"]')
self.assertEqual(tags["list5"], "[]")
self.assertEqual(tags["list6"], "[]")
self.assertEqual(tags["tuple1"], '["a"]')
self.assertEqual(tags["tuple2"], '["a"]')
self.assertEqual(tags["tuple3"], '["2"]')
self.assertEqual(tags["tuple4"], '["2"]')
self.assertEqual(tags["tuple5"], "[]")
self.assertEqual(tags["tuple6"], "[]")
self.assertEqual(tags["range1"], '["0"]')
self.assertEqual(tags["range2"], '["0"]')

exporter = ZipkinSpanExporter(service_name, max_tag_value_length=9)
mock_post = MagicMock()
with patch("requests.post", mock_post):
mock_post.return_value = MockResponse(200)
status = exporter.export([span])
self.assertEqual(SpanExportResult.SUCCESS, status)

_, kwargs = mock_post.call_args # pylint: disable=E0633
tags = json.loads(kwargs["data"])[0]["tags"]
self.assertEqual(len(tags["string1"]), 9)
self.assertEqual(len(tags["string2"]), 9)
self.assertEqual(tags["list1"], '["a"]')
self.assertEqual(tags["list2"], '["a"]')
self.assertEqual(tags["list3"], '["2"]')
self.assertEqual(tags["list4"], '["2"]')
self.assertEqual(tags["list5"], '["True"]')
self.assertEqual(tags["list6"], '["True"]')
self.assertEqual(tags["tuple1"], '["a"]')
self.assertEqual(tags["tuple2"], '["a"]')
self.assertEqual(tags["tuple3"], '["2"]')
self.assertEqual(tags["tuple4"], '["2"]')
self.assertEqual(tags["tuple5"], '["True"]')
self.assertEqual(tags["tuple6"], '["True"]')
self.assertEqual(tags["range1"], '["0"]')
self.assertEqual(tags["range2"], '["0"]')

exporter = ZipkinSpanExporter(service_name, max_tag_value_length=10)
mock_post = MagicMock()
with patch("requests.post", mock_post):
mock_post.return_value = MockResponse(200)
status = exporter.export([span])
self.assertEqual(SpanExportResult.SUCCESS, status)

_, kwargs = mock_post.call_args # pylint: disable=E0633
tags = json.loads(kwargs["data"])[0]["tags"]
self.assertEqual(len(tags["string1"]), 10)
self.assertEqual(len(tags["string2"]), 10)
self.assertEqual(tags["list1"], '["a", "a"]')
self.assertEqual(tags["list2"], '["a", "a"]')
self.assertEqual(tags["list3"], '["2", "2"]')
self.assertEqual(tags["list4"], '["2", "2"]')
self.assertEqual(tags["list5"], '["True"]')
self.assertEqual(tags["list6"], '["True"]')
self.assertEqual(tags["tuple1"], '["a", "a"]')
self.assertEqual(tags["tuple2"], '["a", "a"]')
self.assertEqual(tags["tuple3"], '["2", "2"]')
self.assertEqual(tags["tuple4"], '["2", "2"]')
self.assertEqual(tags["tuple5"], '["True"]')
self.assertEqual(tags["tuple6"], '["True"]')
self.assertEqual(tags["range1"], '["0", "1"]')
self.assertEqual(tags["range2"], '["0", "1"]')

exporter = ZipkinSpanExporter(service_name, max_tag_value_length=11)
mock_post = MagicMock()
with patch("requests.post", mock_post):
mock_post.return_value = MockResponse(200)
status = exporter.export([span])
self.assertEqual(SpanExportResult.SUCCESS, status)

_, kwargs = mock_post.call_args # pylint: disable=E0633
tags = json.loads(kwargs["data"])[0]["tags"]
self.assertEqual(len(tags["string1"]), 11)
self.assertEqual(len(tags["string2"]), 11)
self.assertEqual(tags["list1"], '["a", "a"]')
self.assertEqual(tags["list2"], '["a", "a"]')
self.assertEqual(tags["list3"], '["2", "2"]')
self.assertEqual(tags["list4"], '["2", "2"]')
self.assertEqual(tags["list5"], '["True"]')
self.assertEqual(tags["list6"], '["True"]')
self.assertEqual(tags["tuple1"], '["a", "a"]')
self.assertEqual(tags["tuple2"], '["a", "a"]')
self.assertEqual(tags["tuple3"], '["2", "2"]')
self.assertEqual(tags["tuple4"], '["2", "2"]')
self.assertEqual(tags["tuple5"], '["True"]')
self.assertEqual(tags["tuple6"], '["True"]')
self.assertEqual(tags["range1"], '["0", "1"]')
self.assertEqual(tags["range2"], '["0", "1"]')

# pylint: disable=too-many-locals,too-many-statements
def test_export_protobuf(self):
Expand Down