Adding support for array attributes to Zipkin exporter by robwknox · Pull Request #1285 · open-telemetry/opentelemetry-python

robwknox · 2020-10-26T22:07:55Z

Description

Adds support for basic sequence (list, tuple, range) attribute values in the Zipkin exporter.

Since the value is serialized to a JSON list string, specific logic is needed to ensure max_tag_value_length is honored at the element boundary.

As an aside, I believe I've identified a bug in that bool objects are supposed to be serialized to true or false to adhere to JSON spec, but we appear to be retaining python formatting of True and False. Will file a separate bug report.

Fixes #1110

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

tox environments:

py35-test-exporter-zipkin
py36-test-exporter-zipkin
py37-test-exporter-zipkin
py38-test-exporter-zipkin
pypy3-test-exporter-zipkin

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

…y#1110)

exporter/opentelemetry-exporter-zipkin/CHANGELOG.md

exporter/opentelemetry-exporter-zipkin/src/opentelemetry/exporter/zipkin/__init__.py

aabmass · 2020-12-01T18:11:11Z

exporter/opentelemetry-exporter-zipkin/src/opentelemetry/exporter/zipkin/__init__.py

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


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)

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.

aabmass · 2020-12-01T18:25:36Z

exporter/opentelemetry-exporter-zipkin/src/opentelemetry/exporter/zipkin/__init__.py

+                        running_string_length += (
+                            2  # accounts for ', ' connector
+                        )
+                    if running_string_length > self.max_tag_value_length:


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

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

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?

exporter/opentelemetry-exporter-zipkin/tests/test_zipkin_exporter.py

…thub.com/robwknox/opentelemetry-python into 1110_zipkin_exporter_support_array_attrs

…elements versus bailing completely out

aabmass

As an aside, I believe I've identified a bug in that bool objects are supposed to be serialized to true or false to adhere to JSON spec, but we appear to be retaining python formatting of True and False. Will file a separate bug report.

👍

Nice and thanks for addressing comments! LGTM

ocelotl

LGTM, just leaving some suggestions.

ocelotl · 2020-12-10T21:05:30Z

exporter/opentelemetry-exporter-zipkin/src/opentelemetry/exporter/zipkin/__init__.py

+                value = self._extract_tag_value_string_from_sequence(
+                    attribute_value
+                )
+                if not value:


Suggested change

if not value:

if value is None:

ocelotl · 2020-12-10T21:32:28Z

exporter/opentelemetry-exporter-zipkin/tests/test_zipkin_exporter.py

+        )
+        self.assertEqual(
+            tags["list5"],
+            '["True","True","True","True","True","True","True","True","True","True","True","True","True","True","True","True","True","True"]',


Suggested change

'["True","True","True","True","True","True","True","True","True","True","True","True","True","True","True","True","True","True"]',

partial_dump([str(True)] * 25)

ocelotl · 2020-12-10T21:36:23Z

exporter/opentelemetry-exporter-zipkin/tests/test_zipkin_exporter.py

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


To avoid these long testing sequences, may I suggest:

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

ocelotl · 2020-12-10T21:37:50Z

exporter/opentelemetry-exporter-zipkin/tests/test_zipkin_exporter.py

+        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"]',


Suggested change

'["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"]',

partial_dumps(["a"] * 25),

ocelotl · 2020-12-10T21:40:42Z

exporter/opentelemetry-exporter-zipkin/tests/test_zipkin_exporter.py

+        )
+        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","23","24"]',


Suggested change

'["0","1","2","3","4","5","6","7","8","9","10","11","12","13","14","15","16","17","18","19","20","21","22","23","24"]',

partial_dumps([str(i) for i in range(25)]),

…r_support_array_attrs

codeboten

LGTM

Adding support for array attributes to Zipkin exporter (open-telemetr…

c10311a

…y#1110)

robwknox requested review from a team, aabmass and codeboten and removed request for a team October 26, 2020 22:07

robwknox added 3 commits October 26, 2020 16:47

lint changes (open-telemetry#1110)

83a868c

Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs

8d3f204

CHANGELOG entry (open-telemetry#1110)

779b090

robwknox mentioned this pull request Nov 3, 2020

Zipkin exporter should support Array attributes #1110

Closed

robwknox changed the title ~~Adding support for array attributes to Zipkin exporter (#1110)~~ Adding support for array attributes to Zipkin exporter Nov 3, 2020

codeboten added release:required-for-ga To be resolved before GA release 1.0.0rc2 release candidate 2 for tracing GA labels Nov 5, 2020

lzchen reviewed Nov 12, 2020

View reviewed changes

exporter/opentelemetry-exporter-zipkin/CHANGELOG.md Outdated Show resolved Hide resolved

robwknox and others added 5 commits November 13, 2020 15:45

Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs

b31c881

changelog

1033a76

lint

1d3a63f

Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs

95dfae4

Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs

0082859

lzchen assigned ocelotl Nov 19, 2020

robwknox and others added 3 commits November 23, 2020 13:04

Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs

d82fb30

Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs

a2f1c8d

Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs

75edd7d

aabmass reviewed Dec 1, 2020

View reviewed changes

robwknox added 6 commits December 2, 2020 15:22

review refactor

014f5e4

review refactor

2a5c4b3

Merge branch '1110_zipkin_exporter_support_array_attrs' of https://gi…

85ef28c

…thub.com/robwknox/opentelemetry-python into 1110_zipkin_exporter_support_array_attrs

black

51d8024

black

a226e19

simplify tag value formatting, plus gracefully skip invalid sequence …

907ead5

…elements versus bailing completely out

aabmass approved these changes Dec 10, 2020

View reviewed changes

ocelotl approved these changes Dec 10, 2020

View reviewed changes

Alex Boten added 2 commits December 14, 2020 08:45

Merge remote-tracking branch 'origin/master' into 1110_zipkin_exporte…

32d68e8

…r_support_array_attrs

Merge branch 'master' into 1110_zipkin_exporter_support_array_attrs

7fc3ac9

codeboten approved these changes Dec 16, 2020

View reviewed changes

codeboten merged commit 9cfe555 into open-telemetry:master Dec 16, 2020

	'["True","True","True","True","True","True","True","True","True","True","True","True","True","True","True","True","True","True"]',
	partial_dump([str(True)] * 25)

	'["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"]',
	partial_dumps(["a"] * 25),

	'["0","1","2","3","4","5","6","7","8","9","10","11","12","13","14","15","16","17","18","19","20","21","22","23","24"]',
	partial_dumps([str(i) for i in range(25)]),

Conversation

robwknox commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aabmass left a comment

Choose a reason for hiding this comment

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeboten left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

robwknox commented Oct 26, 2020 •

edited

Loading