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.

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

Allow None in sequence attributes values (#998) #1256

Conversation

LetzNico
Copy link
Contributor

Description

As per the spec, None values in attribute sequences are valid

null values within arrays MUST be preserved as-is (i.e., passed on to spanprocessors / exporters as null). If exporters do not support exporting null values, they MAY replace those values by 0, false, or empty strings.
This is required for map/dictionary structures represented as two arrays with indices that are kept in sync (e.g., two attributes header_keys and header_values, both containing an array of strings to represent a mapping header_keys[i] -> header_values[i]).

Fixes #998

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Unit tests have been added

Checklist:

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

@LetzNico LetzNico requested review from a team, owais and aabmass and removed request for a team October 18, 2020 10:08
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 18, 2020

CLA Check
The committers are authorized under a signed CLA.

@LetzNico LetzNico marked this pull request as draft October 18, 2020 10:12
@LetzNico LetzNico marked this pull request as ready for review October 18, 2020 10:26
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

This looks good, just a couple of non-blocking suggestions

opentelemetry-sdk/tests/trace/test_trace.py Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved
@LetzNico LetzNico force-pushed the 998-set-attribute-with-invalid-value-pops-attribute branch 2 times, most recently from b0ff611 to 71b6174 Compare October 19, 2020 17:19
@LetzNico
Copy link
Contributor Author

Erk, a step failed. It looks like a flaky Flask test. I cannot get CircleCI to relaunch the pipeline tho...

Mon, 19 Oct 2020 17:20:57 GMT test_flask.py::TestFlask::test_flask 
Mon, 19 Oct 2020 17:20:57 GMT -------------------------------- live log call ---------------------------------
Mon, 19 Oct 2020 17:20:57 GMT WARNING  urllib3.connectionpool:connectionpool.py:752 Retrying (Retry(total=9, connect=None, read=None, redirect=None, status=None)) after connection broken by 'NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f270bae84a8>: Failed to establish a new connection: [Errno 111] Connection refused',)': /
Mon, 19 Oct 2020 17:20:59 GMT WARNING  urllib3.connectionpool:connectionpool.py:752 Retrying (Retry(total=8, connect=None, read=None, redirect=None, status=None)) after connection broken by 'NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f270bae8978>: Failed to establish a new connection: [Errno 111] Connection refused',)': /
Mon, 19 Oct 2020 17:20:59 GMT FAILED                                                                   [ 50%]
Mon, 19 Oct 2020 17:20:59 GMT test_tracing.py::TestBasicTracerExample::test_basic_tracer PASSED        [100%]
Mon, 19 Oct 2020 17:20:59 GMT
Mon, 19 Oct 2020 17:20:59 GMT =================================== FAILURES ===================================
Mon, 19 Oct 2020 17:20:59 GMT _____________________________ TestFlask.test_flask _____________________________
Mon, 19 Oct 2020 17:20:59 GMT
Mon, 19 Oct 2020 17:20:59 GMT self = <tests.test_flask.TestFlask testMethod=test_flask>
Mon, 19 Oct 2020 17:20:59 GMT
Mon, 19 Oct 2020 17:20:59 GMT     def test_flask(self):
Mon, 19 Oct 2020 17:20:59 GMT         dirpath = os.path.dirname(os.path.realpath(__file__))
Mon, 19 Oct 2020 17:20:59 GMT         server_script = "{}/../flask_example.py".format(dirpath)
Mon, 19 Oct 2020 17:20:59 GMT         server = subprocess.Popen(
Mon, 19 Oct 2020 17:20:59 GMT             [sys.executable, server_script], stdout=subprocess.PIPE,
Mon, 19 Oct 2020 17:20:59 GMT         )
Mon, 19 Oct 2020 17:20:59 GMT         retry_strategy = Retry(total=10, backoff_factor=1)
Mon, 19 Oct 2020 17:20:59 GMT         adapter = HTTPAdapter(max_retries=retry_strategy)
Mon, 19 Oct 2020 17:20:59 GMT         http = requests.Session()
Mon, 19 Oct 2020 17:20:59 GMT         http.mount("http://", adapter)
Mon, 19 Oct 2020 17:20:59 GMT     
Mon, 19 Oct 2020 17:20:59 GMT         try:
Mon, 19 Oct 2020 17:20:59 GMT             result = http.get("http://localhost:5000")
Mon, 19 Oct 2020 17:20:59 GMT >           self.assertEqual(result.status_code, 200)
Mon, 19 Oct 2020 17:20:59 GMT E           AssertionError: 500 != 200

@LetzNico LetzNico force-pushed the 998-set-attribute-with-invalid-value-pops-attribute branch 2 times, most recently from be4efc9 to 621a18e Compare October 19, 2020 19:25
@codeboten codeboten added the release:required-for-ga To be resolved before GA release label Oct 20, 2020
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM, just need to update the Attributes type

Sequence[str],
Sequence[bool],
Sequence[int],
Sequence[float],

@LetzNico LetzNico force-pushed the 998-set-attribute-with-invalid-value-pops-attribute branch 2 times, most recently from ede3d8b to 6365549 Compare October 24, 2020 10:58
@LetzNico LetzNico force-pushed the 998-set-attribute-with-invalid-value-pops-attribute branch from 6365549 to 206b5c1 Compare October 25, 2020 10:06
@LetzNico LetzNico requested a review from aabmass October 25, 2020 10:31
@codeboten codeboten merged commit ddf7eeb into open-telemetry:master Oct 26, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga To be resolved before GA release
Projects
None yet
4 participants