fix bug in BoundedList for python 3.4 and add tests #199

mauriciovasquezbernal · 2019-10-03T13:06:37Z

While implementing the jaeger exporter I found that it's not possible to iterate over the links of a Span in python3.4 because the BoundedList uses deque.copy() that was introduced in python3.5.
This PR fixes that and adds few tests to avoid similar problems in the future.

collections.deque.copy() was introduced in python 3.5, this commit changes that by the deque constructor and adds some tests to BoundedList and BoundedDict to avoid similar problems in the future.

Oberon00 · 2019-10-03T13:43:52Z

opentelemetry-sdk/tests/test_util.py

+
+
+class TestBoundedList(unittest.TestCase):
+    base = [52, 36, 53, 29, 54, 99, 56, 48, 22, 35, 21, 65, 10, 95, 42, 60]


Why not just use list(range(n))?

I am not that happy having base[i] == i.

Oberon00 · 2019-10-03T13:44:59Z

opentelemetry-sdk/tests/test_util.py

+
+        # sequence too big
+        with self.assertRaises(ValueError):
+            BoundedList.from_seq(list_len / 2, TestBoundedList.base)


You could also add an assertion that modifying base does not modify the BoundedList or vice versa

Oberon00 · 2019-10-03T13:46:17Z

opentelemetry-sdk/tests/test_util.py

+        self.assertEqual(len(blist), 0)
+
+        # fill list
+        for idx in TestBoundedList.base:


Suggested change

for idx in TestBoundedList.base:

for item in TestBoundedList.base:

Oberon00 · 2019-10-03T13:49:55Z

opentelemetry-sdk/tests/test_util.py

+
+        self.assertEqual(len(bdict), dic_len)
+
+        for key in TestBoundedDict.base:


You could also test that all keys are yielded by the iterator like asserting that len(tuple(bdict)) == len(bdict)). Same for BoundedList.

Oberon00 · 2019-10-03T13:52:29Z

opentelemetry-sdk/tests/test_util.py

+            bdict["new-key" + key] = TestBoundedDict.base[key]
+
+        self.assertEqual(len(bdict), dic_len)
+        self.assertEqual(bdict.dropped, dic_len)


Please also assert that the new key is in the bounded dict (i.e., old elements are dropped instead of new ones).
Same for BoundedList.

Good one, actually I was thinking that the new elements were dropped.

could this behavior be documented in the docstring as well?

- assert that changing the base list/dict does not change the BoundedList/BoundedDict - some variables renaming - test that dropped elements are the old ones

toumorokoshi

minor comments, but tests look awesome!

toumorokoshi · 2019-10-04T16:25:37Z

opentelemetry-sdk/tests/test_util.py

+class TestBoundedList(unittest.TestCase):
+    base = [52, 36, 53, 29, 54, 99, 56, 48, 22, 35, 21, 65, 10, 95, 42, 60]
+
+    def test_raises(self):


could docstrings be added to clarify the behavior expected? minor thing but I find terse unit test names hard to quickly scan to better understand the expected behavior.

toumorokoshi · 2019-10-04T16:26:23Z

opentelemetry-sdk/tests/test_util.py

+        blist.append(13)
+
+        with self.assertRaises(IndexError):
+            _ = blist[2]


is this assignment necessary? wouldn't getitem get called regardless of what is done to the return value?

I guess this way it's more explicit that we don't care about the value instead of having made a typo.

pylint will complain with W0104: Statement seems to have no effect (pointless-statement) if _ = is removed.

toumorokoshi · 2019-10-04T16:27:31Z

opentelemetry-sdk/tests/test_util.py

+            _ = blist[-3]
+
+    def test_from_seq(self):
+        list_len = len(TestBoundedList.base)


why not "self.base" here? I worry about calling out the class directly as it just increase the work when one wants to change the class name.

toumorokoshi · 2019-10-04T16:31:59Z

opentelemetry-sdk/tests/test_util.py

+
+        # test __iter__ in BoundedList
+        idx = 0
+        for val in blist:


could use enumerate() here instead of manually incrementing.

toumorokoshi · 2019-10-04T16:33:08Z

opentelemetry-sdk/tests/test_util.py

+        blist = BoundedList.from_seq(list_len, TestBoundedList.base)
+
+        # try to append more items
+        for idx in range(list_len):


I think this could just enumerate the list itself, instead of using an index.

Actually a plain for in self.base would work here.

toumorokoshi · 2019-10-04T16:33:36Z

opentelemetry-sdk/tests/test_util.py

+
+        # test __iter__ in BoundedList
+        idx = 0
+        for val in blist:


could use enumerate

toumorokoshi · 2019-10-04T16:35:59Z

opentelemetry-sdk/tests/test_util.py

+            bdict["new-key" + key] = TestBoundedDict.base[key]
+
+        self.assertEqual(len(bdict), dic_len)
+        self.assertEqual(bdict.dropped, dic_len)


could this behavior be documented in the docstring as well?

- improve docstrings of testing cases - use self.base instead of <ClassName>.base - improve docstrings of BoundedList and BoundedDict classes - use enumerate() where possible

Oberon00

I think this PR is a fine improvement already. Any further improvements, if deemed worthy, can be done in a separate PR.

Oberon00 · 2019-10-07T09:08:45Z

opentelemetry-sdk/tests/test_util.py

+            self.assertEqual(blist[idx], self.base[idx])
+
+        # test __iter__ in BoundedList
+        for idx, val in enumerate(blist):


You could even use

Suggested change

for idx, val in enumerate(blist):

for val1, val2 in zip(blist, self.base):

here.

for start_time and end_time Make lint happy Addressing comments Addressing comments Allowing 0 as start and end time Fix lint issues Metrics API RFC 0003 cont'd (open-telemetry#136) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * address comments * fix comments Adding a working propagator, adding to integrations and example (open-telemetry#137) Adding a full, end-to-end example of propagation at work in the example application, including a test. Adding the use of propagators into the integrations. Metrics API RFC 0009 (open-telemetry#140) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * handle, recordbatch * docs * Update recordbatch * black * Fix typo * remove ValueType * fix lint Console exporter (open-telemetry#156) Make use_span more flexible (closes open-telemetry#147). (open-telemetry#154) Co-Authored-By: Reiley Yang <reyang@microsoft.com> Co-Authored-By: Chris Kleinknecht <libc@google.com> WSGI fixes (open-telemetry#148) Fix http.url. Don't delay calling wrapped app. Skeleton for azure monitor exporters (open-telemetry#151) Add link to docs to README (open-telemetry#170) Move example app to the examples folder (open-telemetry#172) WSGI: Fix port 80 always appended in http.host (open-telemetry#173) Build and host docs via github action (open-telemetry#167) Add missing license boilerplate to a few files (open-telemetry#176) sdk/trace/exporters: add batch span processor exporter (open-telemetry#153) The exporters specification states that two built-in span processors should be implemented, the simple processor span and the batch processor span. This commit implements the latter, it is mainly based on the opentelemetry/java one. The algorithm implements the following logic: - a condition variable is used to notify the worker thread in case the queue is half full, so that exporting can start before the queue gets full and spans are dropped. - export is called each schedule_delay_millis if there is a least one new span to export. - when the processor is shutdown all remaining spans are exported. Implementing W3C TraceContext (fixes open-telemetry#116) (open-telemetry#180) * Implementing TraceContext (fixes open-telemetry#116) This introduces a w3c TraceContext propagator, primarily inspired by opencensus. fix time conversion bug (open-telemetry#182) Introduce Context.suppress_instrumentation (open-telemetry#181) Metrics Implementation (open-telemetry#160) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * handle, recordbatch * docs * Update recordbatch * black * Fix typo * remove ValueType * fix lint * sdk * metrics * example * counter * Tests * Address comments * ADd tests * Fix typing and examples * black * fix lint * remove override * Fix tests * mypy * fix lint * fix type * fix typing * fix tests * isort * isort * isort * isort * noop * lint * lint * fix tuple typing * fix type * black * address comments * fix type * fix lint * remove imports * default tests * fix lint * usse sequence * remove ellipses * remove ellipses * black * Fix typo * fix example * fix type * fix type * address comments Implement Azure Monitor Exporter (open-telemetry#175) Span add override parameters for start_time and end_time (open-telemetry#179) CONTRIBUTING.md: Fix clone URL (open-telemetry#177) Add B3 exporter to alpha release table (open-telemetry#164) Update README for alpha release (open-telemetry#189) Update Contributing.md doc (open-telemetry#194) Add **simple** client/server examples (open-telemetry#191) Remove unused dev-requirements.txt (open-telemetry#200) The requirements are contained in tox.ini now. Fx bug in BoundedList for Python 3.4 and add tests (open-telemetry#199) * fix bug in BoundedList for python 3.4 and add tests collections.deque.copy() was introduced in python 3.5, this commit changes that by the deque constructor and adds some tests to BoundedList and BoundedDict to avoid similar problems in the future. Also, improve docstrings of BoundedList and BoundedDict classes Move util.time_ns to API. (open-telemetry#205) Add Jaeger exporter (open-telemetry#174) This adds a Jeager exporter for OpenTelemetry. This exporter is based on https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-jaeger. The exporter uses thrift and can be configured to send data to the agent and also to a remote collector. There is a long discussion going on about how to include generated files in the repo, so for now just put them here. Add code coverage Revert latest commit Fix some "errors" found by mypy. (open-telemetry#204) Fix some errors found by mypy (split from open-telemetry#201). Update README for new milestones (open-telemetry#218) Refactor current span handling for newly created spans. (open-telemetry#198) 1. Make Tracer.start_span() simply create and start the Span, without setting it as the current instance. 2. Add an extra Tracer.start_as_current_span() to create the Span and set it as the current instance automatically. Co-Authored-By: Chris Kleinknecht <libc@google.com> Add set_status to Span (open-telemetry#213) Initial commit Initial version

fix bug in BoundedList for python 3.4 and add tests

ec22a67

collections.deque.copy() was introduced in python 3.5, this commit changes that by the deque constructor and adds some tests to BoundedList and BoundedDict to avoid similar problems in the future.

mauriciovasquezbernal requested review from a-feld, c24t, carlosalberto, lzchen, Oberon00, reyang and toumorokoshi as code owners October 3, 2019 13:06

Oberon00 approved these changes Oct 3, 2019

View reviewed changes

Oberon00 reviewed Oct 3, 2019

View reviewed changes

mauriciovasquezbernal added 2 commits October 3, 2019 12:21

handle feedback

897ba7c

- assert that changing the base list/dict does not change the BoundedList/BoundedDict - some variables renaming - test that dropped elements are the old ones

make lint happy

038c21e

toumorokoshi approved these changes Oct 4, 2019

View reviewed changes

handle review

4579754

- improve docstrings of testing cases - use self.base instead of <ClassName>.base - improve docstrings of BoundedList and BoundedDict classes - use enumerate() where possible

Oberon00 approved these changes Oct 7, 2019

View reviewed changes

Oberon00 merged commit 0d9b7bd into open-telemetry:master Oct 7, 2019

mauriciovasquezbernal deleted the mauricio/test_bounded_list_and_dict branch April 14, 2020 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bug in BoundedList for python 3.4 and add tests #199

fix bug in BoundedList for python 3.4 and add tests #199

mauriciovasquezbernal commented Oct 3, 2019

Oberon00 Oct 3, 2019

mauriciovasquezbernal Oct 3, 2019

Oberon00 Oct 3, 2019

Oberon00 Oct 3, 2019

Oberon00 Oct 3, 2019

Oberon00 Oct 3, 2019

mauriciovasquezbernal Oct 3, 2019

toumorokoshi Oct 4, 2019

toumorokoshi left a comment

toumorokoshi Oct 4, 2019

toumorokoshi Oct 4, 2019

Oberon00 Oct 4, 2019

mauriciovasquezbernal Oct 4, 2019

toumorokoshi Oct 4, 2019

toumorokoshi Oct 4, 2019

toumorokoshi Oct 4, 2019

mauriciovasquezbernal Oct 4, 2019

toumorokoshi Oct 4, 2019

toumorokoshi Oct 4, 2019

Oberon00 left a comment

Oberon00 Oct 7, 2019



		class TestBoundedList(unittest.TestCase):
		base = [52, 36, 53, 29, 54, 99, 56, 48, 22, 35, 21, 65, 10, 95, 42, 60]

	for idx in TestBoundedList.base:
	for item in TestBoundedList.base:


		self.assertEqual(len(bdict), dic_len)

		for key in TestBoundedDict.base:

	for idx, val in enumerate(blist):
	for val1, val2 in zip(blist, self.base):

fix bug in BoundedList for python 3.4 and add tests #199

fix bug in BoundedList for python 3.4 and add tests #199

Conversation

mauriciovasquezbernal commented Oct 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment