search

actions-on-google / actions-on-google-nodejs

Node.js client library for Actions on Google
https://actions-on-google.github.io/actions-on-google-nodejs
Apache License 2.0
899 stars 193 forks source link

Smart Home: Cache authorization tokens #421

Open bramkragten opened 3 years ago

bramkragten commented 3 years ago

For every requestSync or reportState request, a new JWTClient is created, causing a post request to get a new token: https://github.com/actions-on-google/actions-on-google-nodejs/blob/eaa9e2058273dd7832031e1d8496e134ba60f791/src/service/smarthome/smarthome.ts#L332

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 commented 3 years ago

FYI @proppy

proppy commented 3 years ago

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 commented 3 years ago

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

Thanks.

bramkragten commented 3 years ago

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 commented 3 years ago

When would that be good to use in production?

I'd recommend to ask on https://github.com/googleapis/google-api-nodejs-client/issues/1130

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

bramkragten commented 3 years ago

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

proppy commented 3 years ago

@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 https://github.com/actions-on-google/actions-on-google-nodejs/issues/421#issuecomment-819986778.

bramkragten commented 3 years ago

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: https://github.com/actions-on-google/actions-on-google-nodejs/issues/267#issuecomment-450026465.

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

Thanks!

bramkragten commented 3 years ago

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 commented 3 years ago

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

kevin-induro commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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: