[WIP] Implementation of Python Stats API by kcooperstein · Pull Request #149 · census-instrumentation/opencensus-python

kcooperstein · 2018-04-06T22:42:03Z

No description provided.

…thon into stats

googlebot · 2018-04-06T22:42:07Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

kcooperstein · 2018-04-06T22:47:51Z

I signed it!

kcooperstein · 2018-04-06T22:49:26Z

I signed it!

googlebot · 2018-04-06T22:49:29Z

CLAs look good, thanks!

liyanhui1228 · 2018-04-06T22:53:53Z

examples/stats/basic_prototype.py

+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.


Please add this license to all the other files.

Also please use the correct year.

liyanhui1228 · 2018-04-08T17:56:15Z

opencensus/stats/aggregation.py

+    def __init__(self, boundaries=None, distribution=None, aggregation_type="distribution"):
+        super().__init__(aggregation_type, boundaries)
+        self.aggregation_type = aggregation_type
+        '''self.boundaries = list(bucket_boundaries.BucketBoundaries(boundaries) or [])'''


Remove this line?

liyanhui1228 · 2018-04-08T17:56:34Z

opencensus/stats/aggregation.py

+        self.boundaries = bucket_boundaries.BucketBoundaries(boundaries)
+        self.distribution = dict(distribution or {})
+
+    def get_aggregation(self):


In Python we don't really use getters and setters like in Java, because we can get the variable just by calling self.aggregation_type in this case. Usually when we use getter method, we add a @property decorator before the method signature, and the variable the getter returned is a private variable in this class which has a _ before the variable name. Then with this decorated getter method, calling self.aggregation_type will return the value of the private variable self._aggregation_type in this class.

For more details look at this link: https://www.python-course.eu/python3_properties.php

liyanhui1228 · 2018-04-08T17:56:55Z

opencensus/stats/exporters/stackdriver_exporter.py

+        self.resource = client.resource('global', {})
+
+    def emit(self, views, datapoint=None):
+        name = 'projects/{}'.format(self.project_id)


Move this line to __init__ so you don't need to construct the name every time you call emit.

liyanhui1228 · 2018-04-08T17:57:06Z

opencensus/stats/exporters/stackdriver_exporter.py

+
+    def emit(self, views, datapoint=None):
+        name = 'projects/{}'.format(self.project_id)
+        '''metric = {}'''


liyanhui1228 · 2018-04-08T17:57:38Z

setup.py


 install_requires = [
    'google-cloud-trace>=0.17.0, <0.18dev',
+    'google-cloud-monitoring>=0.28.1'


google-cloud-monitoring>=0.28.1, <0.29,

liyanhui1228 · 2018-04-08T17:58:24Z

opencensus/tags/tag_map.py

+        else:
+            return "key is not in map"
+
+


Clean up the extra new lines.

liyanhui1228 · 2018-04-08T17:58:35Z

opencensus/tags/tag_key.py

+            return False
+
+
+


Clean up extra new lines.

liyanhui1228 · 2018-04-08T17:58:57Z

opencensus/stats/exporters/stackdriver_exporter.py

+            metrics[metric_type] = metric_label
+            labels.append(metric_label)
+        return metrics
+


Clean up extra new lines.

liyanhui1228 · 2018-04-08T17:59:33Z

opencensus/stats/exporters/stackdriver_exporter.py

+
+    def translate_to_stackdriver(self, views):
+        metrics = {}
+        labels = []


What's the purpose of labels?

will remove

liyanhui1228 · 2018-04-08T18:00:19Z

opencensus/stats/exporters/stackdriver_exporter.py

+            return 'Successfully wrote time series.'
+
+    def set_resource(self, type_, labels):
+        return self.client.resource(type_, labels)


Set 'global' as the default type_ here.

liyanhui1228 · 2018-04-09T22:14:11Z

opencensus/trace/tracers/__init__.py

 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+from opencensus.trace import tracer as tracer_module


Why changing this file?

liyanhui1228 · 2018-04-11T00:07:31Z

FYI I updated the nox.py config to enable it run on both trace and stats directory, please fix the lint warnings and test coverage.

liyanhui1228 · 2018-04-11T00:13:33Z

opencensus/tags/tag_value.py

+    def value(self):
+        return self._value
+
+    def is_value(self, value):


Rename to check_value?

liyanhui1228 · 2018-04-11T00:30:07Z

opencensus/tags/tag_map.py

+        if key in self.map:
+            return self.map[key]
+        else:
+            return "key is not in map"


If you want to emit a warning message, it should be either in the msg of an exception or a logging msg, but should not be directly returned. Because it will mess up with your intended return type of the tag value. In this case, probably change to something like:

value = self.map.get(key, None) if value is None: raise KeyError('Key is not in map.') return value

liyanhui1228 · 2018-04-11T00:31:42Z

opencensus/tags/tag_map.py

+        if key in self.map:
+            return True
+        else:
+            return False


Simplify this with:

return key in self.map

liyanhui1228 · 2018-04-11T00:35:11Z

opencensus/tags/tag_map.py

+        if map is None:
+            self.map = {}
+        else:
+            self.map = map


self.map = map if map is not None else {}

liyanhui1228 · 2018-04-11T00:36:30Z

opencensus/tags/tag_map.py

+            self.map = map
+        if tags is not None:
+            self.tags = tags
+            print(self.tags)


remove print statement?

liyanhui1228 · 2018-04-11T00:36:36Z

opencensus/tags/tag_map.py

+            self.tags = tags
+            print(self.tags)
+            for tag in self.tags:
+                print(tag)


liyanhui1228 · 2018-04-11T00:37:03Z

opencensus/tags/tag_key.py

+    def name(self):
+        return self._name
+
+    def is_valid(self, name):


Rename to check_name?

liyanhui1228 · 2018-04-11T00:37:33Z

opencensus/tags/tag_context.py

+
+    def get_tag_value(self, key):
+        if key in self._tags:
+            return self._tags[key]


return self._tags.get(key, None)

liyanhui1228 · 2018-04-16T17:05:29Z

venv/include/python2.7

@@ -0,0 +1 @@
+/usr/local/Cellar/python/2.7.14_3/Frameworks/Python.framework/Versions/2.7/include/python2.7


Use this command git rm --cached venv/include/python2.7 to ignore the previously committed file and it will be ignored after that.

Please remove the venv/ files from the commits.

liyanhui1228 · 2018-04-16T17:05:38Z

venv/pip-selfcheck.json

@@ -0,0 +1 @@
+{"last_check":"2018-02-13T00:28:25Z","pypi_version":"9.0.1"}


Same for this one.

Still here?

wkiser

Thanks for the PR! I think it might make sense to split this into a few smaller PRs since there is a lot going on. Maybe start with the base classes + related tests?

wkiser · 2018-04-16T18:37:56Z

opencensus/stats/aggregation.py

+class BaseAggregation(object):
+
+    def __init__(self, aggregation_type=None, buckets=None):
+        if aggregation_type is not None:


aggregation_type can probably be a class level variable since it will be the same for all objects of the type.

wkiser · 2018-04-16T18:41:35Z

opencensus/stats/aggregation.py

+        else:
+            self._aggregation_type = "none"
+
+        if buckets is not None:


I think self._buckets = buckets or [] is more readable, but I'm not sure if that is against this repo's style guide.

wkiser · 2018-04-16T18:44:55Z

opencensus/stats/aggregation.py

+    def __init__(self, count=None, aggregation_type="count"):
+        super().__init__(aggregation_type)
+        self._aggregation_type = aggregation_type
+        if count is not None:


you can make 0 the default value for count in the initializer: count=0,...

wkiser · 2018-04-16T18:48:00Z

opencensus/stats/aggregation_data.py

+        self._count_data = count_data
+
+    def add_sample(self, value):
+        self._count_data = self._count_data + 1


self._count_data += 1

@kcooperstein pls change this.

wkiser · 2018-04-16T18:48:54Z

opencensus/stats/aggregation_data.py

+        self._sum_data = sum_data
+
+    def add_sample(self, value):
+        self._sum_data = self._sum_data + value


self._sum_data += value

wkiser · 2018-04-16T18:50:42Z

opencensus/stats/exporters/stackdriver_exporter.py

+        self.client = client
+        self.project_id = client.project
+        self.resource = client.resource('global', {})
+        name = 'projects/{}'.format(self.project_id)


wkiser · 2018-04-16T18:52:02Z

opencensus/stats/exporters/stackdriver_exporter.py

+            self.client.resource = self.set_resource(type_='global', labels= {'project_id': self.project_id})
+            self.client.write_point(metric, self.client.resource, datapoint, datetime.utcnow() + timedelta(seconds=60), datetime.utcnow())
+            self.client.time_series(metric, self.client.resource, datapoint, datetime.utcnow() + timedelta(seconds=60), datetime.utcnow())
+            return 'Successfully wrote time series.'


I would log this rather than return it.

wkiser · 2018-04-16T18:52:39Z

opencensus/stats/aggregation_data.py

+        return self._count_data
+
+class DistributionAggregationData(BaseAggregationData):
+    def __init__(self, mean_data, count_data, min, max, sum_of_sqd_deviations, counts_per_bucket, bounds):


might want to do min_, max_ since min and max are reserved words

wkiser · 2018-04-16T18:57:39Z

opencensus/stats/exporters/stackdriver_exporter.py

+            self.client.time_series(metric, self.client.resource, datapoint, datetime.utcnow() + timedelta(seconds=60), datetime.utcnow())
+            return 'Successfully wrote time series.'
+
+    def set_resource(self, type_='global', labels={}):


weird python quirk, but it's considered dangerous to use an empty dict as the default value to a function. I would replace with labels=None and create a new empty dict inside the function body is labels is None.

wkiser · 2018-04-16T19:09:39Z

opencensus/tags/tag_map.py

+class TagMap(object):
+
+    def __init__(self, tags=None, map=None):
+        self.map = map if map is not None else {}


can do self.map = map or {}

liyanhui1228 · 2018-05-07T22:36:32Z

Adding @songy23 for sanity check.

liyanhui1228 · 2018-05-07T22:59:28Z

opencensus/stats/measure_to_view_map.py

+        self._registered_views[measure.name] = view
+        if registered_measure is None:
+            self._registered_measures[measure.name] = measure
+        self._map[view.measure.name] = ViewData(view=view, start_time=timestamp, end_time=timestamp)


Adding comment based on our discussion here for future reference: self._map should be implemented as a multi-map instead of a single map, as there could be multiple view data using the same measure. And in this way there will only be one view data because everything before are overwrote.

songy23

First pass - I haven't looked at the tag part.

songy23 · 2018-05-07T22:52:02Z

opencensus/stats/aggregation.py

+        return self._count
+
+
+class MeanAggregation(BaseAggregation):


Mean Aggregation should be removed now: census-instrumentation/opencensus-specs#56.

songy23 · 2018-05-07T22:53:15Z

opencensus/stats/aggregation_data.py

+        return self._count_data
+
+
+class MeanAggregationData(BaseAggregationData):


songy23 · 2018-05-07T22:53:53Z

opencensus/stats/bucket_boundaries.py

+class BucketBoundaries(object):
+
+    def __init__(self, boundaries=None):
+        self._boundaries = list(boundaries or [])


You should also check the boundaries are in ascending order.

songy23 · 2018-05-07T22:56:56Z

opencensus/stats/measure_to_view_map.py

+class MeasureToViewMap(object):
+
+    def __init__(self):
+        self._map = {}


This should be a multi map (Java counterpart).

songy23 · 2018-05-07T23:00:10Z

opencensus/stats/view_data.py

+        self._view = view
+        self._start_time = start_time
+        self._end_time = end_time
+        self._rows = rows if rows is not None else {}


FWIW rows and tag_value_aggregation_map are the same thing.

liyanhui1228

Mostly LGTM excepts the Circle CI needs to be green before we merge. Please fix the lint and coverage tests. And don't forget to remove the unnecessary files like venv/. Use git rm --cached venv/include/python2.7 to remove the committed files from the commits.

songy23

Please consider moving the exporter code to another PR.

songy23 · 2018-05-09T20:53:42Z

opencensus/stats/aggregation_data.py

+    :type count_data: int
+    :param count_data: the count value of the distribution
+
+    :type min_: double


nit: min_ -> min

I believe min is a keyword in python which is why it is not used, would you recommend to use the keyword instead?

Got it, I'm fine with leaving it as-is. I was just wondering why these two were different from other attributes.

songy23 · 2018-05-09T20:53:49Z

opencensus/stats/aggregation_data.py

+    :type min_: double
+    :param min_: the minimum value of the distribution
+
+    :type max_: double


songy23 · 2018-05-09T21:10:57Z

opencensus/tags/tag_key.py

+        :rtype: bool
+        :returns: True if it valid, else returns False
+        """
+        if (len(name) > 0) and (len(name) <= 255):


You should also check if the given string is a ASCII string. See Go and specs.

songy23 · 2018-05-09T21:13:45Z

opencensus/tags/tag_value.py

+        """The current value"""
+        return self._value
+
+    def is_valid_value(self, value):


Same here, you should check if it's an ASCII string.

songy23 · 2018-05-09T21:19:30Z

opencensus/stats/view_data.py

+        return self._rows
+
+    @property
+    def tag_value_aggregation_map(self):


The key of this map should be different combinations of TagValues, rather than single TagValue.

songy23 · 2018-05-09T21:20:09Z

opencensus/stats/view_data.py

+    def record(self, context, value, timestamp):
+        """records the view data against context"""
+        tag_values = self.get_tag_values(tags=self.get_tag_map(context), columns=self.view.columns)
+        for tag_value in tag_values:


Same here, you should check and update entry for unique combination of TagValues instead of going through each one.

songy23 · 2018-05-09T21:22:19Z

opencensus/stats/measure_to_view_map.py

+
+    def get_view(self, view_name, timestamp):
+        """get the View Data from the given View name"""
+        view = self.get_view_data(view_name)


This view is actually a ViewData?

songy23 · 2018-05-09T21:24:35Z

opencensus/stats/measure_to_view_map.py

+        if view is None:
+            return None
+
+        view_data = ViewData(view=view, start_time=timestamp, end_time=timestamp)


IIUC the variable view is a ViewData rather than a View. How do you set the rows/aggregation map for the ViewData?

liyanhui1228 · 2018-05-14T17:50:49Z

opencensus-traces.json

@@ -0,0 +1 @@
+{"traceId": "f8739df974a4481f98748cd92b27177d", "spans": [{"displayName": {"value": "child_span", "truncated_byte_count": 0}, "spanId": "739fba56569f4106", "startTime": "2018-05-07T21:04:16.261253Z", "endTime": "2018-05-07T21:04:18.266410Z", "childSpanCount": 0, "kind": 0, "parentSpanId": "c3e7efa94eb747d3"}, {"displayName": {"value": "root_span", "truncated_byte_count": 0}, "spanId": "c3e7efa94eb747d3", "startTime": "2018-05-07T21:04:14.256056Z", "endTime": "2018-05-07T21:04:18.266438Z", "childSpanCount": 1, "kind": 0, "parentSpanId": "6e0c63257de34c92"}]}


Is this file accidentally committed? Remove it?

Remove this file.

liyanhui1228 · 2018-05-14T17:52:02Z

opencensus/stats/aggregation_data.py

+        self._count_data = count_data
+
+    def add_sample(self, value):
+        self._count_data = self._count_data + 1


@kcooperstein pls change this.

liyanhui1228 · 2018-05-14T17:52:46Z

opencensus/stats/aggregation_data.py

    :param count_data: the count value of the distribution

-    :type min_: double
+    :type min_: min


min is not a type

liyanhui1228 · 2018-05-14T17:52:56Z

opencensus/stats/aggregation_data.py

    :param min_: the minimum value of the distribution

-    :type max_: double
+    :type max_: max


This reverts commit 6e34928, reversing changes made to 6c04ddd.

liyanhui1228

Please fix the final review comments and we'll be good to merge.

liyanhui1228 · 2018-05-15T21:57:58Z

examples/stats/basic_prototype.py

+
+
+
+


Two blank lines between functions.

liyanhui1228 · 2018-05-15T21:58:08Z

opencensus-traces.json

@@ -0,0 +1 @@
+{"traceId": "f8739df974a4481f98748cd92b27177d", "spans": [{"displayName": {"value": "child_span", "truncated_byte_count": 0}, "spanId": "739fba56569f4106", "startTime": "2018-05-07T21:04:16.261253Z", "endTime": "2018-05-07T21:04:18.266410Z", "childSpanCount": 0, "kind": 0, "parentSpanId": "c3e7efa94eb747d3"}, {"displayName": {"value": "root_span", "truncated_byte_count": 0}, "spanId": "c3e7efa94eb747d3", "startTime": "2018-05-07T21:04:14.256056Z", "endTime": "2018-05-07T21:04:18.266438Z", "childSpanCount": 1, "kind": 0, "parentSpanId": "6e0c63257de34c92"}]}


Remove this file.

liyanhui1228 · 2018-05-15T21:59:49Z

venv/.Python

@@ -0,0 +1 @@
+/usr/local/Cellar/python/2.7.14_3/Frameworks/Python.framework/Versions/2.7/Python


Still here?

liyanhui1228 · 2018-05-15T21:59:56Z

venv/pip-selfcheck.json

@@ -0,0 +1 @@
+{"last_check":"2018-02-13T00:28:25Z","pypi_version":"9.0.1"}


Still here?

liyanhui1228 · 2018-05-15T22:53:25Z

We can ignore the CLA, merging...

kcooperstein added 12 commits March 19, 2018 13:46

Merge remote-tracking branch 'census-instrumentation/master'

a984d1e

Creating stats "bare bone" prototype

e84352a

Creating stats "bare bone" prototype

9536d65

Merge branch 'stats' of https://github.com/kcooperstein/opencensus-py…

a15996f

…thon into stats

Updating Stats commit with updated exporter and prototype

b4d3090

Fixing exporter based on git comments

dd5edea

Updating Stats commit with updated exporter and prototype

eaa5df3

Merge remote-tracking branch 'origin/stats' into stats

2d00eca

Updating aggregation, aggregation data, and creating tag map

7246f47

Merge remote-tracking branch 'upstream/master'

bb0fc03

[WIP] Implementation of Python Stats API

dc93a2f

Merge branch 'master' into stats

ee4b0dc

kcooperstein closed this Apr 6, 2018

tags07 reopened this Apr 6, 2018

liyanhui1228 self-requested a review April 6, 2018 22:51

liyanhui1228 reviewed Apr 6, 2018

View reviewed changes

liyanhui1228 suggested changes Apr 8, 2018

View reviewed changes

liyanhui1228 reviewed Apr 9, 2018

View reviewed changes

kcooperstein and others added 3 commits April 9, 2018 18:46

[WIP] Updating Implementation of Python Stats API

02bb85d

Merge branch 'master' into stats

26ec5fe

Merge branch 'master' into stats

308d218

liyanhui1228 suggested changes Apr 11, 2018

View reviewed changes

[WIP] Updating Python Stats Library and adding testing

43ebb73

liyanhui1228 reviewed Apr 16, 2018

View reviewed changes

wkiser reviewed Apr 16, 2018

View reviewed changes

liyanhui1228 reviewed May 7, 2018

View reviewed changes

songy23 reviewed May 7, 2018

View reviewed changes

kcooperstein and others added 2 commits May 8, 2018 20:08

Updating documentation and testing

8431230

Merge branch 'master' into stats

1e667ea

liyanhui1228 approved these changes May 9, 2018

View reviewed changes

songy23 reviewed May 9, 2018

View reviewed changes

liyanhui1228 and others added 2 commits May 11, 2018 16:53

Merge branch 'master' into stats

be90589

Fixing tests for lint warnings

4719e71

liyanhui1228 reviewed May 14, 2018

View reviewed changes

kcooperstein added 11 commits May 14, 2018 11:14

Fixing tests for lint warnings

da8cf4a

Merge remote-tracking branch 'origin/stats' into stats

8833fe9

Merge remote-tracking branch 'origin/stats' into stats

89a5c8a

Fixing aggregation data and view data

6c04ddd

Merge remote-tracking branch 'origin/stats' into stats

6e34928

Revert "Merge remote-tracking branch 'origin/stats' into stats"

8d04eb4

This reverts commit 6e34928, reversing changes made to 6c04ddd.

Fixing coverage tests

768717a

Fixing coverage tests

0a8a9e8

Fixing coverage tests

da395da

Merge remote-tracking branch 'origin/stats' into stats

8276b46

Removing venv files

30a50cb

liyanhui1228 reviewed May 15, 2018

View reviewed changes

Removing venv files

39a3aaa

liyanhui1228 merged commit 8e4ce16 into census-instrumentation:master May 15, 2018

c24t mentioned this pull request Oct 30, 2018

Multiple apparent bugs in DistributionAggregation implementation #373

Closed

c24t mentioned this pull request Nov 7, 2018

Remove unused DistributionAggregation fields #388

Open

colincadams mentioned this pull request Nov 8, 2018

Readme mentions webapp2 integration with no details #392

Closed

c24t mentioned this pull request Jul 2, 2019

Support int typed aggregations #696

Merged

		@@ -0,0 +1 @@
		/usr/local/Cellar/python/2.7.14_3/Frameworks/Python.framework/Versions/2.7/include/python2.7 No newline at end of file

		@@ -0,0 +1 @@
		{"last_check":"2018-02-13T00:28:25Z","pypi_version":"9.0.1"} No newline at end of file

		return self._count_data


		class MeanAggregationData(BaseAggregationData):

		@@ -0,0 +1 @@
		{"traceId": "f8739df974a4481f98748cd92b27177d", "spans": [{"displayName": {"value": "child_span", "truncated_byte_count": 0}, "spanId": "739fba56569f4106", "startTime": "2018-05-07T21:04:16.261253Z", "endTime": "2018-05-07T21:04:18.266410Z", "childSpanCount": 0, "kind": 0, "parentSpanId": "c3e7efa94eb747d3"}, {"displayName": {"value": "root_span", "truncated_byte_count": 0}, "spanId": "c3e7efa94eb747d3", "startTime": "2018-05-07T21:04:14.256056Z", "endTime": "2018-05-07T21:04:18.266438Z", "childSpanCount": 1, "kind": 0, "parentSpanId": "6e0c63257de34c92"}]} No newline at end of file

Conversation

kcooperstein commented Apr 6, 2018

Uh oh!

googlebot commented Apr 6, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

kcooperstein commented Apr 6, 2018

Uh oh!

kcooperstein commented Apr 6, 2018

Uh oh!

googlebot commented Apr 6, 2018

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

liyanhui1228 commented Apr 11, 2018

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!

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!