Set X-Planet-App header based on origin of request

[sarasafavi]

Closes #336, #339

A quick-n-dirty attempt to set "X-Planet-App" custom header to either planet-sdk or planet-cli, depending on origin of the request (acceptable values of this header are planet-sdk, planet-cli, or unknown). Defaults to planet-sdk for non-CLI-originated Sessions.

[sarasafavi]

notably - this doesn't yet set an origin for Subscriptions, to avoid conflicting with any imminent changes

[jreiberkyle]

looks great and my only comments are more food for thought around how the header will be used than anything that requires change

[jreiberkyle]

This is a kind way of handling an unexpected value for origin. What about just kicking up a ClientError if origin isn't one of the expected values?

[sarasafavi]

We could, but as this value is solely for logging purposes I think failing permissively is fine. If a user for some reason initiates a Session and deliberately changes origin to an unexpected value, I'd be curious about the reason but not enough to block with a ClientError :)

[jreiberkyle]

If the origin is CLI, do we want the python prepended? Probably depends on how we intend to use it. From the user's point of view, a CLI is a CLI and the only python part about it is how they install it.

[sarasafavi]

the python-xxx format is just following precedent in the current logged values (e.g., V1 is/was logged as python-client). We'll ultimately want to be able to log future tools (e.g., a hypothetical r-sdk) too, so this gives us room to grow towards that.

[jreiberkyle]

slick solution :)

[sgillies]

@sarasafavi @jreiberkyle my concern is that we're adding a new argument to the Session constructor with no guidance to external users about whether they can choose their own values. It could be a red herring?

If we would like to know if a Session is created in the context of a CLI command or not, we can look at the interpreter frame stack. It's a bit esoteric, but is reliable for CPython (not PyPy). For example, I copied this to the end of planet.models.Session.__init__().

        import inspect
        caller = inspect.stack()[1]
        LOGGER.info("Caller function=%r, filename=%r", caller.function, caller.filename)

and then when I run python -m pytest tests/integration/test_subscriptions_cli.py -k create_failure -rP --log-level INFO I see

INFO     planet.http:http.py:138 Caller function='create_subscription_cmd', filename='/home/seangillies/projects/planet-client-python/planet/cli/subscriptions.py'

There we go, the Session constructor was called by create_subscription_cmd from planet/cli/subscriptions.py. In that case we could add "cli" to the header. I like this because it doesn't increase the Session API surface and doesn't require any new documentation. It won't work for PyPy because that Python runtime doesn't have frames, but in that case we could stick with "sdk" in the header.

[sgillies]

I have another idea, much simpler than my previous one and with all the same benefits (no Session API expansion). We subclass Session for use under planet/cli and set the user agent and header there.

class CliSession(Session):

    def __init__(self, auth: AuthType = None):
        super().__init__(auth)
        self._client.headers.update({
            'User-Agent': f'planet-sdk-python/{__version__}',
            'X-Planet-App': 'python-cli'
        })

[sgillies]

Superseded by #607 after discussion with @sarasafavi .

[jreiberkyle]

Ohh yes subclassing is a great approach!

…

On Jun 28, 2022, at 1:01 PM, Sean Gillies ***@***.***> wrote:  I have another idea, much simpler. We subclass Session for use under planet/cli and set the user agent and header there. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.