Skip to content
This repository has been archived by the owner on May 28, 2024. It is now read-only.

Smart Home: Cache authorization tokens #421

Open
bramkragten opened this issue Apr 12, 2021 · 14 comments · May be fixed by #423
Open

Smart Home: Cache authorization tokens #421

bramkragten opened this issue Apr 12, 2021 · 14 comments · May be fixed by #423

Comments

@bramkragten
Copy link

bramkragten commented Apr 12, 2021

For every requestSync or reportState request, a new JWTClient is created, causing a post request to get a new token:

const jwtClient = new google.auth.JWT(

The token that is returned is valid for an hour, but the client and token are still recreated for every request.

I suggest creating the JWTClient just once, on first use, this causes that the token is cached and only refreshed when it is no longer valid.

Furthermore, during the request of a new token, all other requests should wait for this token to finish loading instead of starting a new load.

This will drastically improve the performance and lower the number of requests to the Google Authentication API.

I can create a PR if this is approved.

@Fleker
Copy link
Member

Fleker commented Apr 13, 2021

FYI @proppy

bramkragten added a commit to bramkragten/actions-on-google-nodejs that referenced this issue Apr 13, 2021
@bramkragten bramkragten linked a pull request Apr 13, 2021 that will close this issue
@proppy
Copy link
Contributor

proppy commented Apr 15, 2021

Hi @bramkragten,

Thanks for the report (and the PR!).

A few updates to share on our side:
A dedicated homegraph client was released to NPM a few days ago, https://www.npmjs.com/package/@googleapis/homegraph. Being based on the Google APIs Node.js client this package should give you full control over the caching and the invalidation of the credentials, see: https://googleapis.dev/nodejs/googleapis/latest/homegraph/index.html

Additionally this client is generated from the Homegraph API discovery document https://homegraph.googleapis.com/$discovery/rest (itself being derived from the googleapis homegraph protocol buffers: https://github.com/googleapis/googleapis/blob/master/google/home/graph/v1/homegraph.proto) which ensure that it will be kept up to date with every APIs changes.

Given this, we're in the process of deprecating the homegraph wrapper methods in actions-on-google-nodejs in favor of recommending that developers use https://www.npmjs.com/package/@googleapis/homegraph directly.

Would that be an acceptable solution for you? Does it solves the concerns you had with #421 and #422 ?

@bramkragten
Copy link
Author

Yes, that looks like a much better configurable client.
I implemented it now, will check and compare the performance.

Thanks.

@bramkragten
Copy link
Author

bramkragten commented Apr 15, 2021

Btw, would you know the state of HTTP/2 support? https://github.com/googleapis/google-api-nodejs-client#http2

When would that be good to use in production?

@proppy
Copy link
Contributor

proppy commented Apr 15, 2021

When would that be good to use in production?

I'd recommend to ask on googleapis/google-api-nodejs-client#1130

What's the concurrent numbers of requests you're looking at?

@bramkragten
Copy link
Author

bramkragten commented Apr 15, 2021

We currently do 200-400 requests per second, with 200 max concurrent.

@proppy
Copy link
Contributor

proppy commented Apr 15, 2021

@bramkragten then you may have better result using the Home Graph API gRPC surface documented at https://developers.google.com/assistant/smarthome/reference/rpc/google.home.graph.v1#google.home.graph.v1.HomeGraphApiService.

I just created a self-answered question on SO that shows basic usage with Node.js: https://stackoverflow.com/questions/67113632/how-to-call-the-home-graph-api-with-grpc-on-node-js/67113633#67113633

But depending on the language you're using you should be able to generate bindings for any languages supported by gRPC using the public protobuf definitions at: https://github.com/googleapis/googleapis/blob/master/google/home/graph/v1/homegraph.proto

Note: I'll leave this issue and your PR open until we proceed with the deprecations plan highlighted in #421 (comment).

@bramkragten
Copy link
Author

Our implementation was using gRPC, but it was hacky and not really maintainable.

We actually just migrated away from gRPC, because it was advised in this repo to use this package for its simplified API and performance should not differ much: #267 (comment).

I will look at your example, it looks pretty nice.

Thanks!

@bramkragten
Copy link
Author

To report back on this:

The @googleapis/homegraph package gave around the same performance as with my patched version (#423), a lot more CPU intensive than our old gRPC implementation, but functional.

Switching back to gRPC using your SO answer has lowered our CPU usage to roughly our old level again, +/- 4 times lower than using HTTP.

@proppy
Copy link
Contributor

proppy commented Apr 21, 2021

@bramkragten good to hear, what about resource usage (# of socket opened, memory)?

@kevin-induro
Copy link

@proppy
I've had this library implemented for a while with very simple configuration. Basically, I put the service account in functions config, pass it to the smarthome function, and call reportSync/requestState when I need to. For example,

const jwt = functions.config().smarthome.service_account;
const smarthomeClient = smarthome({ jwt });
smarthomeClient.requestSync(uid);

Based on what I see in your SO post, you're suggesting the path forward is a much more complicated implementation using lower level APIs that each developer must maintain themselves. What's the incentive here?

Perhaps a better question, why not migrate the current wrapper to using the new dedicated HomeGraph client instead of deprecating everything?

@proppy
Copy link
Contributor

proppy commented Sep 9, 2021

@kevin-induro sorry for the confusion, the SO post was meant to answers @bramkragten asks about using gRPC to multiplex request to the Home Graph API.

The current recommended client for calling the Home Graph API from Node.js is https://www.npmjs.com/package/@googleapis/homegraph, which is automatically (and regularly!) re-generated from the Home Graph API definition. The API complexity should be close to the current wrapper while allowing to access missing functionality (like requestSync async flag).

See the updated Node.js snippets at:

@kevin-induro
Copy link

Ah! That makes a lot of sense. Sorry, I haven't looked back at the docs since it's been implemented for a while.

Looking at the docs now, it seems to indicate that we'll now have to have both libraries, then. This one for fulfillment and that one for everything else, no?

@proppy
Copy link
Contributor

proppy commented Sep 13, 2021

Looking at the docs now, it seems to indicate that we'll now have to have both libraries, then. This one for fulfillment and that one for everything else, no?

Yep, there are multiple options to handle fulfillment and communicate with homegraph:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants