logging: convert to gax

[stephenplusplus]

RE: #1859

To Dos

• Convert from gRPC to gax
  • Handle automatic project ID insertion
  • Logging
  • Log
  • Metadata
  • Sink
• Tests
  • Unit
    • Logging
    • Log
    • Metadata
    • Sink
  • System

This converts the Logging handwritten API from making manual gRPC requests via GrpcService to gax.

[stephenplusplus]

@jmuk -- I'm running into an issue with the listLogEntries method. I believe I'm calling it correctly, but I'm getting an error from the API.

{ orderBy: 'timestamp desc',
  pageSize: 1,
  projectIds: [ 'nth-circlet-705' ] }

Insufficient tokens for quota 'ReadGroup' and limit 'CLIENT_PROJECT-100s' of service 'logging.googleapis.com' for consumer 'project_number:288560394597'.

Do you know what I need to do to fix this?

[jmuk]

The error message is saying about the quota, somehow the API calls could hit to some limit? I'm still not sure, but it might be worth trying autoPaginate: false option to check if the error happens during automatic paginations.

Also, probably nicer to use resourceNames rather than projectIds -- the projectIds are marked as deprecated now. However I don't think resourceNames/projectIds difference affects the errors like this.

[stephenplusplus]

autoPaginate: false gets rid of the error.

[jmuk]

Oh and you specified pageSize as 1, so it probably goes into a number of API calls which comes to the quota for the number of API calls (https://cloud.google.com/logging/quota-policy 10 per seconds per project).

[stephenplusplus]

That's a bug in the generated code, right? API requests shouldn't continue after I receive my pageSize limit, regardless of autoPaginate value. This is done in @google-cloud/common paginator: https://github.com/GoogleCloudPlatform/google-cloud-node/blob/c51f14c7af7c01a3bdc520c180e767ea0448f660/packages/common/src/paginator.js#L210 (prior to that function executing, pageSize is translated to maxResults).

I can see how pageSize could mean "make as many requests as needed, but don't give me more than pageSize back per response". However, that's a library-agnostic option, i.e. a user familiar with the Logging API has no idea what autoPaginate is. So, how could the API have made the assumption we would put auto-paginating behavior in front of it? I think the expected behavior is to treat it as maxResults, and set autoPaginate to false automatically. Does that make sense?

[jmuk]

I think this is an intended behavior. pageSize parameter specifies the number of the elements in a single API response object, I'm thinking it does not imply the auto pagination. auto paginations will be controlled by the autoPaginate flag.

[jmuk]

But: if it's quite common among GCN, probably we should change the behavior on the codegen side. Is there good examples of the same pattern on other APIs?

[stephenplusplus]

It looks like it's just Logging that uses pageSize, and it doesn't look like there is an alternative option for maxResults behavior. Since this is the only API, we can re-assign what pageSize means here. If it makes more sense to use it as the API defines it, we can switch to that. Let me know what you think we should do.

[stephenplusplus]

@jmuk autoPaginate seems to be ignored in streaming APIs. That might make sense, since it's a stream, but it's leading to a "run until it dies" scenario, because the API eventually shuts me down due to my quota being exceeded. I think some type of maxResults argument should be supported by the generated API, wdyt?

[stephenplusplus]

It also seems that I can't end a stream early. We support this in GCN with the help of split-array-stream. This will take a chunk of API results, emit them onto a stream, and then execute a callback with an argument stating whether the stream has been closed or not: https://github.com/stephenplusplus/split-array-stream/blob/master/index.js

Before each item is pushed to the stream, it also makes sure the stream isn't ended. In this scenario, this would allow me to do something like this:

var entriesMatched = 0
logging.getEntriesStream()
  .on('data', function() {
    entriesMatched++
    if (entriesMatched === 20) this.end()
    if (entriesMatched > 20) throw new Error('I should only get 20!')
  })
  .on('end', function() {
    assert.strictEqual(entriesMatched, 20)
  })

maxResults would make the above scenario more logical, but ending a stream before all results have been received is useful as well, in the event you're looking for a specific item, and once you've found it, you don't want any more connections open or requests to be made.

[stephenplusplus]

@callmehiphop PTAL -- some things to consider:

We use the {{projectId}} token replacement-- GAX has no idea about that, and doesn't have anything similar. So we have to stand in front of all GAX requests and replace those tokens.

We can get the authClient from GAX, but, we can't instantiate GAX unless we have a projectId or it throws. So, that's why I included google-auto-auth to get the auth client & projectId, and why I lazily instantiate the GAX clients.

@jmuk marked this as blocked because without being able to end a stream, a method like getEntriesStream(), which can have a very large number of results, is prone to breaking the QPS limits.

[jmuk]

Sorry for the delays:

• maxResults might be a good idea, that can happen anyways when the number of logs grow.
• not ending lists properly is a bug on google-gax library. To make sure, that's for the paged responses like listLogEntriesStream method, correct?

[jmuk]

will be tracked by googleapis/google-cloud-node-core#493 and googleapis/google-cloud-node-core#492.

[stephenplusplus]

Yes, anywhere a stream is used and it has the chance to make multiple requests. split-array-stream might be worth looking into. I made it for this exact case -- you can see the usage example in its readme is based on our use case

Also, this got collapsed by GH, but is also a blocker, as we are getting output printed to the screen: #2072 (comment)

Thanks!

[stephenplusplus]

@jmuk just to get an idea, how far off are these issues from being worked on? Feel free to let me know if you have any further questions or need a second pair of eyes on anything. Thanks!

[callmehiphop]

@stephenplusplus a sort of general thought on the implementation, we do something similar in Spanner where we have to step in front of most requests to make sure we have a session available first and I think the implementations are a little bit different (here we use strings, there we use a bound function).

Thoughts on a small refactor for the sake of consistency?

[stephenplusplus]

Unfortunately, I have to go with the string (and we'll have to do this in Spanner... and Speech... and Vision as well), because we can't instantiate the generated client until we have a project ID.

[callmehiphop]

@stephenplusplus ah, right. LGTM then

[stephenplusplus]

@jmuk can you ping when the new release is available? Will new files need to be generated?

[jmuk]

published google-gax 0.13.0. This includes the fixes and maxResults parameter.

[stephenplusplus]

Awesome, thanks!

[coveralls]

Changes Unknown when pulling 51a81f3 on stephenplusplus:spp--1859-logging into ** on GoogleCloudPlatform:master**.

[coveralls]

Changes Unknown when pulling 81e88ce on stephenplusplus:spp--1859-logging into ** on GoogleCloudPlatform:master**.

[stephenplusplus]

@callmehiphop ready for another look-- tests have been added, but I believe the source is the same as your last review.

[callmehiphop]

LGTM