DISCUSSION PR: Add environ to gcloud package to augment user agent #573

dhermes · 2015-01-27T06:31:47Z

This should not be merged (uses lots of pragma: NO COVER), it is just a quick and dirty way to go about it.

The App Engine import is innocuous, but the Compute Engine metadata server HTTP should not happen every single time gcloud is imported (or any sub-package / sub-module). This is the thing I want to discuss.

coveralls · 2015-01-27T06:34:21Z

Changes Unknown when pulling cf58724 on dhermes:fix-566 into * on GoogleCloudPlatform:master*.

coveralls · 2015-01-28T00:08:19Z

Changes Unknown when pulling 5324510 on dhermes:fix-566 into * on GoogleCloudPlatform:master*.

tseaver · 2015-01-28T17:11:14Z

Making an HTTP request at import time is Just Wrong (TM). We should either make an instance of a class with a property which computes __environ__ lazily (maybe __version__ too?), or else ditch the dunder-variable and use a helper function (which could cache the result in a private module global).

dhermes · 2015-01-28T17:13:27Z

I like it. Will re-work with this in mind. I'm a little scared of what unit tests will look like.

Temporary commit for PR googleapis#573.

coveralls · 2015-01-29T22:16:13Z

Coverage decreased (-0.02%) to 99.98% when pulling 1d4c4de on dhermes:fix-566 into d858ff0 on GoogleCloudPlatform:master.

dhermes · 2015-01-29T22:23:28Z

@tseaver I changed to using a lazy loading property with __str__ defined. I have yet to write tests because I want to discuss (made bogus pragma no cover to avoid).

I also debated using a default USER_AGENT on the class and then on Connection instance creation set a specific user agent? This could be difficult in tests if we create a lot of Connections (I'm not sure if we do).

Why can't GCE be detected without an HTTP request? Bah!

I'm a bit hesitant with this current approach since we have a if self._user_agent is None check before every single HTTP request. This is making me lean back towards setting at import time; the 0.1 second timeout will make it virtually imperceptible.

Another thing I wanted to do: set gcloud-python/0.3.0-regression-test for regression tests. (This can be done without this PR.)

tseaver · 2015-01-29T22:27:20Z

Something like Pyramid's @reify decorator eliminates the lookup overhead.

tseaver · 2015-01-29T22:31:47Z

Maybe we should allow applications to pass in a string to be added to the agent?

dhermes · 2015-01-29T22:35:10Z

I like the reify approach, I'll go down that path and ping you again.
Allow them to pass a string, that sounds good. I debated just leaving this methods and asking people to set themselves, but realized people don't care about the user-agent. The reason we (or Google) would want the environ set is so we can get more information about how our library is used in the wild.

dhermes · 2015-01-29T23:55:28Z

@tseaver PTAL. I held off on

allowing a custom user agent
setting one during regression tests

Will cover in another PR if you're OK with that.

coveralls · 2015-01-29T23:58:02Z

Coverage remained the same at 100.0% when pulling 2f3f5dc on dhermes:fix-566 into d858ff0 on GoogleCloudPlatform:master.

dhermes · 2015-01-29T23:59:43Z

Also I had to add a pragma: NO COVER on the use of the reify decorator. Rather than marked as a partial branch, running coverage html results in it being marked as exit. @tseaver any idea what that means?

coveralls · 2015-01-30T01:28:42Z

Coverage remained the same at 100.0% when pulling b926608 on dhermes:fix-566 into d858ff0 on GoogleCloudPlatform:master.

gcloud/connection.py

@@ -45,6 +39,11 @@ def __init__(self, credentials=None):
        self._http = None
        self._credentials = credentials

+    @gcloud._UserAgentReifyProperty  # pragma: NO COVER


tseaver · 2015-01-30T01:47:22Z

Deferring the "custom" agent stuff is fine by me.

tseaver · 2015-01-30T01:50:16Z

Or move the logic down into the user_agent method and have the decorator just cache the result of calling in in the instance dict.

dhermes · 2015-01-30T01:54:21Z

We want to leave the logic outside of Connection so that

branching stops occurring after first call (hence the reify pattern)
Connection can re-use the environ found without having to make another HTTP request

I'm fine with hard-coding user_agent as the name or with living the pragma: NO COVER.

tseaver · 2015-01-30T17:01:25Z

I was thinking more of something like this gist.

dhermes · 2015-01-30T17:10:33Z

Is there something inherently wrong with the way I've currently constructed it?
Also, what is app_id doing in the gist? I'm not sure if a unique user agent per user is a bad thing? (i.e. I don't know what tool Google uses to determine where traffic comes from)
What happens to Connection.user_agent here? It seems people should be able to just hack it with a string if they like. This ends up requiring something like Connection.user_agent.user_agent

tseaver · 2015-01-30T17:23:48Z

Is there something inherently wrong with the way I've currently constructed it?

Using a decorator to wrap a no-op function smells bad to me.

What is app_iddoing in the gist?

I was addressing #573 (comment). If there were "well known" apps which used the feature, then one might be able to pick their traffic out of the logs.

What happens to Connection.user_agent here?

I had assumed that it would just use the value of gcloud_environment.user_agent (which would be the instance holding the cached / computed value.

dhermes · 2015-01-30T17:29:14Z

Yes I think wrapping the no-op smells too, I'd rather just hard code the property name as user_agent (it's already a big clue by the name of the class)
I figured DISCUSSION PR: Add __environ__ to gcloud package to augment user agent #573 (comment) would be more free form
Assigning Connection.user_agent = gcloud_environment.user_agent at definition time will cause reify to make the HTTP request and we're back in the same situation of an HTTP during import

tseaver · 2015-01-30T17:38:37Z

I figured #573 (comment) would be more free form.

It seemed useful to me to preserve the "library" info along with the application-specific tag.

Assigning Connection.user_agent = gcloud_environment.user_agent at definition time will cause reify to make the HTTP request and we're back in the same situation of an HTTP during import.

I hadn't meant to force it at import time; rather, I would use @reify to wrap Connection.user_agent().

dhermes · 2015-01-30T20:55:34Z

Where do you envision this class living?
RE: preserving UA string, I was planning to add a kwarg to Connection and just append (as your gist does)
Did you mean to have gcloud_environment = GCloudEnvironment() (instead of the class) in your gist?
Do we actually want the GCloudEnvironment class to be public, or just for demonstration purposes?

Using the property on Connection instances instead of setting as a class property / attribute. Fixes googleapis#566.

This prevents unnecessary HTTP requests to determine if GCE is the current environment. Also updating Test__UserAgentReifyProperty with - setUp and tearDown to make sure a fresh _UserAgentReifyProperty is associated with the Connection class in every test case - test___get___access_twice test case added to test the branches in __get__ for pre-cache and post-cache states

dhermes · 2015-02-05T02:08:44Z

@tseaver Rebased on top of #580. Can we resume the conversation?

coveralls · 2015-02-05T02:09:38Z

Coverage remained the same at 100.0% when pulling fd08c42 on dhermes:fix-566 into 9518db7 on GoogleCloudPlatform:master.

tseaver · 2015-02-05T20:44:39Z

Where do you envision this class living?

gcloud/environment.py?

RE: preserving UA string, I was planning to add a kwarg to Connection and just append (as your gist does)

Did you mean to have gcloud_environment = GCloudEnvironment() (instead of the class) in your gist?

Yes: the whole notion requires an instance to hold the cached computed values.

Do we actually want the GCloudEnvironment class to be public, or just for demonstration purposes?

I don't care about the class, per se. I was assuming that the instance might be part of the API, assuming apps might care about using its methods in the same way that the connection does.

* chore: Update gapic-generator-python to v1.11.2 PiperOrigin-RevId: 546510849 Source-Link: googleapis/googleapis@736073a Source-Link: googleapis/googleapis-gen@deb64e8 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZGViNjRlOGVjMTlkMTQxZTMxMDg5ZmU5MzJiM2E5OTdhZDU0MWM0ZCJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 27, 2015

dhermes added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 27, 2015

dhermes force-pushed the fix-566 branch from cf58724 to 5324510 Compare January 28, 2015 00:04

dhermes force-pushed the fix-566 branch 2 times, most recently from a53d2c8 to 1d4c4de Compare January 29, 2015 22:13

dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Jan 29, 2015

Bogus test coverage avoidance via pragma: NO COVER.

1d4c4de

Temporary commit for PR googleapis#573.

dhermes force-pushed the fix-566 branch from 1d4c4de to 2f3f5dc Compare January 29, 2015 23:54

dhermes force-pushed the fix-566 branch from 2f3f5dc to b926608 Compare January 30, 2015 01:26

dhermes removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 30, 2015

tseaver reviewed Jan 30, 2015
View reviewed changes

gcloud/connection.py

@@ -45,6 +39,11 @@ def __init__(self, credentials=None):

self._http = None

self._credentials = credentials

@gcloud._UserAgentReifyProperty # pragma: NO COVER

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

dhermes added 2 commits February 4, 2015 18:02

Adding lazy loading user agent property to main package.

f2c29b4

Using the property on Connection instances instead of setting as a class property / attribute. Fixes googleapis#566.

dhermes force-pushed the fix-566 branch from b926608 to fd08c42 Compare February 5, 2015 02:07

tseaver mentioned this pull request Feb 6, 2015

Tracking status of Transaction as well as success / failure. #595

Merged

dhermes added the api: core label Feb 14, 2015

dhermes mentioned this pull request Feb 18, 2015

Adding support for GCD dev server in datastore package. #656

Merged

dhermes closed this Feb 19, 2015

dhermes deleted the fix-566 branch February 19, 2015 04:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DISCUSSION PR: Add environ to gcloud package to augment user agent #573

DISCUSSION PR: Add environ to gcloud package to augment user agent #573

dhermes commented Jan 27, 2015

coveralls commented Jan 27, 2015

coveralls commented Jan 28, 2015

tseaver commented Jan 28, 2015

dhermes commented Jan 28, 2015

coveralls commented Jan 29, 2015

dhermes commented Jan 29, 2015

tseaver commented Jan 29, 2015

tseaver commented Jan 29, 2015

dhermes commented Jan 29, 2015

dhermes commented Jan 29, 2015

coveralls commented Jan 29, 2015

dhermes commented Jan 29, 2015

coveralls commented Jan 30, 2015

This comment was marked as spam.

This comment was marked as spam.

tseaver commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

dhermes commented Feb 5, 2015

coveralls commented Feb 5, 2015

tseaver commented Feb 5, 2015

DISCUSSION PR: Add __environ__ to gcloud package to augment user agent #573

DISCUSSION PR: Add __environ__ to gcloud package to augment user agent #573

Conversation

dhermes commented Jan 27, 2015

coveralls commented Jan 27, 2015

coveralls commented Jan 28, 2015

tseaver commented Jan 28, 2015

dhermes commented Jan 28, 2015

coveralls commented Jan 29, 2015

dhermes commented Jan 29, 2015

tseaver commented Jan 29, 2015

tseaver commented Jan 29, 2015

dhermes commented Jan 29, 2015

dhermes commented Jan 29, 2015

coveralls commented Jan 29, 2015

dhermes commented Jan 29, 2015

coveralls commented Jan 30, 2015

This comment was marked as spam.

This comment was marked as spam.

tseaver commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

dhermes commented Feb 5, 2015

coveralls commented Feb 5, 2015

tseaver commented Feb 5, 2015

DISCUSSION PR: Add environ to gcloud package to augment user agent #573

DISCUSSION PR: Add environ to gcloud package to augment user agent #573