Stats core changes by MarceloAquino7 · Pull Request #256 · census-instrumentation/opencensus-python

MarceloAquino7 · 2018-08-14T13:49:08Z

This PR contains all changes inside stats core in order to support exporters and the new transport class.

googlebot · 2018-08-14T13:49:15Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

songy23 · 2018-08-14T17:18:04Z

Please fix the CLA.

A quick fix would be to squash all the commits into one: (assume upstream is https://github.com/census-instrumentation/opencensus-python.git, and origin is your personal repo)

$ git fetch upstream
$ git rebase -i upstream/master
$ # In the rebase UI, pick the top one commit and mark all others as "squash" or "s". Save and exit.
$ # Resolve any potential merge conflicts.
$ # Should see: Successfully rebased and updated refs/heads/master.
$ git push -f origin

Removing stats exporter stuff Removing unused imports Minor changes in comments and copyright Unit tests for LastValue aggregation

googlebot · 2018-08-14T17:42:51Z

CLAs look good, thanks!

MarceloAquino7 · 2018-08-14T17:45:24Z

@songy23 I think that everything is fine right now.

odeke-em · 2018-08-19T21:14:49Z

opencensus/common/transports/async.py

+	def _get_items(self):
+		"""Get multiple items from a Queue.
+
+		Gets at least one (blocking) and at most ``max_items`` items


max_items isn't defined anywhere. Did we perhaps mean to use max_batch_size?

Yes, it was a mistake inside the comment. I'll fix that, thanks !

odeke-em · 2018-08-19T21:16:22Z

opencensus/common/transports/async.py

+		Pulls pending data off the queue and writes them in
+		batches to the specified tracing backend using the exporter.
+		"""
+		print('Background thread started.')


Perhaps this was a legacy print but it pollutes user programs, please remove it or perhaps make it a log that can be turned on

odeke-em · 2018-08-19T21:18:47Z

opencensus/common/transports/async.py

+			if quit_:
+				break
+
+		print('Background thread exited.')


Ditto about prints polluting user code from the library.

odeke-em · 2018-08-19T21:20:06Z

opencensus/common/transports/async.py

+		thread some time to finish processing before this function returns.
+
+		:type grace_period: float
+		:param grace_period: If specified, this method will block up to this


grace_period is mentioned as a param but I don't see it in the function signature.

Right now we are reading its value inside the constructor, so of course do not make sense this piece of comment. I'll remove it, thanks !

odeke-em · 2018-08-19T21:22:56Z

opencensus/common/transports/async.py

+		self._queue.put_nowait(datas)
+
+	def flush(self):
+		"""Submit any pending datas."""


s/datas/data/g, data is the plural form of datum as per https://www.merriam-webster.com/dictionary/data :)

odeke-em · 2018-08-19T21:24:01Z

opencensus/stats/aggregation.py

      SUM (int): The aggreation type of the view is 'sum'.
      COUNT (int): The aggreation type of the view is 'count'.
      DISTRIBUTION (int): The aggreation type of the view is 'distribution'.
+      LASTVALUE (int): The aggreation type of the view is 'lastvalue'.


Minor nit: s/aggreation/aggregation/g

odeke-em

Nice, thanks @MarceloAquino7! I'll wait for @liyanhui1228 to take a look too and I'll also give the entire PR a full read a few more times. Thanks for incorporating the changes for the transport into opencensus.common.transports.*

liyanhui1228

Mostly LGTM, just some style nits. And please also fix the test coverage.

liyanhui1228 · 2018-08-17T23:16:37Z

opencensus/stats/aggregation.py

+        """
+            The current recorded value
+        """
+        return self._value


Need an extra blank line at the end of file.

liyanhui1228 · 2018-08-17T23:17:44Z

opencensus/stats/aggregation_data.py

+        """
+            The current value recorded
+        """
+        return self._value


Need extra blank line.

liyanhui1228 · 2018-08-21T17:06:44Z

opencensus/common/transports/async.py

+
+
+class _Worker(object):
+	"""A background thread that exports batches of data.


The indentation of this file is different than other files in this repo, shall we change to use 4 spaces indentation to match with the repo?

liyanhui1228 · 2018-08-21T17:12:50Z

opencensus/common/transports/base.py

+
+        For blocking/sync transports, this is a no-op.
+        """
+


Remove extra blank line.

Thanks for your reply @liyanhui1228 , I will work on these items. About the test coverage, do you guys use 4 spaces for indentation ?

Yes we use 4 spaces indentation.

…tructure

odeke-em · 2018-08-24T22:33:08Z

@liyanhui1228 they've updated the code with our requests, PTAL again :)

@MarceloAquino7 please rebase from master, as Github currently reports some conflicts with master.

liyanhui1228 · 2018-08-24T22:52:19Z

This LGTM, will approve once the CI is green.

odeke-em · 2018-08-24T22:55:01Z

@liyanhui1228 oh cool, I've updated the branch and now CI is running, please check back in say 10 minutes for when CI returns.

odeke-em · 2018-08-24T23:13:35Z

@liyanhui1228 for some reason CircleCI tripped claiming that the coverage for some test failed

nox > Command coverage report --show-missing --fail-under=100 failed with exit code 2

https://circleci.com/gh/census-instrumentation/opencensus-python/616?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link PTAL

liyanhui1228 · 2018-08-24T23:14:49Z

Yes we should add more test to fix that test coverage.

c24t · 2019-02-21T05:57:53Z

tests/unit/common/transports/test_sync.py

+            'spans': [{}, {}],
+        }
+        transport.export(data)
+        self.assertTrue(True)


What happened here?

googlebot added the cla: no label Aug 14, 2018

Squashing commits

f839e21

Removing stats exporter stuff Removing unused imports Minor changes in comments and copyright Unit tests for LastValue aggregation

MarceloAquino7 force-pushed the stats_core_changes branch from 03462db to f839e21 Compare August 14, 2018 17:42

googlebot added cla: yes and removed cla: no labels Aug 14, 2018

songy23 mentioned this pull request Aug 14, 2018

Stackdriver stats exporter #250

Merged

odeke-em reviewed Aug 19, 2018

View reviewed changes

odeke-em requested a review from liyanhui1228 August 19, 2018 21:30

Few adjustments due to Emmanuel's review

f89b60a

liyanhui1228 reviewed Aug 21, 2018

View reviewed changes

MarceloAquino7 added 5 commits August 22, 2018 15:43

Fix due to Angela's reply | Created unit test for the new transport s…

ed6f363

…tructure

Renaming file

4ec6dc3

Sync transport unit test

15f687f

Lint fixes

9397a8a

Lint fixes

750fd30

Merge branch 'master' into stats_core_changes

1edfeba

songy23 mentioned this pull request Aug 25, 2018

plans to support stats #280

Closed

MarceloAquino7 and others added 3 commits August 27, 2018 11:10

Add tests for situations with exporter registered

123fc3b

Merge branch 'master' into stats_core_changes

ecaacc3

Merge branch 'master' into stats_core_changes

1359da2

songy23 approved these changes Aug 28, 2018

View reviewed changes

liyanhui1228 approved these changes Aug 28, 2018

View reviewed changes

liyanhui1228 merged commit eface0a into census-instrumentation:master Aug 28, 2018

songy23 mentioned this pull request Aug 29, 2018

Stats exemplars #287

Merged

odeke-em mentioned this pull request Sep 22, 2018

quickstart/python: complete stats tutorial census-instrumentation/opencensus-website#354

Closed

c24t mentioned this pull request Nov 6, 2018

DRY up duplicate transport code #386

Closed

c24t reviewed Feb 21, 2019

View reviewed changes



		class _Worker(object):
		"""A background thread that exports batches of data.

Conversation

MarceloAquino7 commented Aug 14, 2018

Uh oh!

googlebot commented Aug 14, 2018

Uh oh!

songy23 commented Aug 14, 2018

Uh oh!

googlebot commented Aug 14, 2018

Uh oh!

MarceloAquino7 commented Aug 14, 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!

odeke-em left a comment

Choose a reason for hiding this comment

Uh oh!

liyanhui1228 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

odeke-em commented Aug 24, 2018

Uh oh!

liyanhui1228 commented Aug 24, 2018

Uh oh!

odeke-em commented Aug 24, 2018

Uh oh!

odeke-em commented Aug 24, 2018

Uh oh!

liyanhui1228 commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liyanhui1228 left a comment •

edited

Loading