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

[dhermes]

See #566.

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]

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

[coveralls]

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

[tseaver]

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]

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

[coveralls]

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

[dhermes]

@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]

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

[tseaver]

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

[dhermes]

1. I like the reify approach, I'll go down that path and ping you again.
2. 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]

@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]

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

[dhermes]

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]

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

[tseaver]

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

[tseaver]

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]

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]

I was thinking more of something like this gist.

[dhermes]

• 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]

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]

• 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]

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]

• 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?

[dhermes]

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

[coveralls]

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

[tseaver]

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.