Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logging: use gRPC #1134

Merged
merged 1 commit into from
Mar 21, 2016
Merged

Conversation

stephenplusplus
Copy link
Contributor

RE: #1049

This transitions our Logging API to use gRPC.

To Dos

  • Tests
    • System
    • Unit
      • GrpcService
      • Logging
        • entry
        • index
        • log
        • sink

⚠️ Breaking Change!

- log.createWriteStream();

Writing to a log from a stream is gone. gRPC (the library) doesn't see WriteLogEntries as a stream.

@stephenplusplus stephenplusplus added don't merge api: logging Issues related to the Cloud Logging API. labels Feb 18, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 18, 2016
@jgeewax
Copy link
Contributor

jgeewax commented Feb 18, 2016

/cc @jayantkolhe

@murgatroid99
Copy link

On the gRPC side, we are working on static Node.js code generation using protoc, and I expect that to be complete within a few weeks. Since this is a first party solution, we would prefer that Google APIs use it to publish client libraries.

This will have a different API than the current dynamically generated code does: instead of passing and receiving bare JavaScript objects, users will build and use objects defined in protobuf-specific classes. This will allow for more immediate and informative input type-checking, and it will be more consistent with other languages.

As a side note, gRPC doesn't see WriteLogEntries as a stream because it is not declared as a stream in the proto file. protoc would see the same thing.

@stephenplusplus
Copy link
Contributor Author

Thanks! I'm sorry to have to admit, but I'm not quite sure where to begin to realize that vision. Is there any documentation, example code, guides, or reading material you can point me to?

@murgatroid99
Copy link

The pull request protocolbuffers/protobuf#1215 contains a significant update to JavaScript protobuf code generation and documentation, so that is probably a good place to start.

@stephenplusplus
Copy link
Contributor Author

So does this sound right:?

  1. Added support for CommonJS require() protocolbuffers/protobuf#1215 merges
  2. I use the CLI tool protoc to feed in proto files from the cloud services. We get a generated JS file back?
  3. We give that generated file to gRPC, which returns an API that this library will interact with
  4. That API won't be similar to what it is now... "...users will build and use objects defined in protobuf-specific classes" - can you elaborate on this?

@murgatroid99
Copy link

More accurately:

  1. That PR merges
  2. On the gRPC side, we create a plugin for protoc that generates service files. This is the part that will take a few weeks.
  3. You use the CLI tool protoc to feed in proto files and get generated JS service and method definition files.
  4. You require('generated_service.js') (which has its own require('grpc') internally) and you get something very similar to the output of grpc.load('service.proto').

Regarding the API change: currently, the objects you pass as arguments and receive as responses in Node gRPC are regular JavaScript objects that are converted on the fly to or from ProtoBuf.js Message objects. With this new generated code, users will instead pass in and receive protobuf Message objects of the relevant types.

@stephenplusplus
Copy link
Contributor Author

Thanks for clearing that up.

With this new generated code, users will instead pass in and receive protobuf Message objects of the relevant types.
This will allow for more immediate and informative input type-checking, and it will be more consistent with other languages.

Can't mismatched input types from a plain JavaScript object be caught by gRPC at runtime?

@stephenplusplus stephenplusplus added the status: blocked Resolving the issue is dependent on other work. label Feb 18, 2016
@murgatroid99
Copy link

Can't mismatched input types from a plain JavaScript object be caught by gRPC at runtime?

It can, but currently that check is done when the message is written. If we require users to pass protobuf Message objects, they will get that type checking when they are building the message objects.

@stephenplusplus
Copy link
Contributor Author

In our case, those events will be in succession, since I think we want to continue to allow plain objects as input for our API. Our library would have to do a lot of plumbing just to convert those objects to Message objects, so having that handled for us has been quite a help.

@stephenplusplus
Copy link
Contributor Author

@murgatroid99 is there a PR or issue I can subscribe to for the protoc plugin that generates service files?

@murgatroid99
Copy link

The issue is at grpc/grpc#5418

@stephenplusplus
Copy link
Contributor Author

I'm going to move forward with the goal of getting this released as-is before the end of the month. After that, we'll iterate where necessary to align with the best practices around gRPC.

@murgatroid99
Copy link

I have a PR for this at grpc/grpc#5189. I believe I can have this complete and submitted within the next few days; definitely by the end of the week.

@stephenplusplus
Copy link
Contributor Author

Sorry if I'm just missing the connection, but did you mean grpc/grpc#5445?

@murgatroid99
Copy link

Yes, that's right. That was just a copy/paste error

@stephenplusplus stephenplusplus force-pushed the spp--1049-logging branch 7 times, most recently from 2975a8a to 10a1480 Compare March 15, 2016 17:41
@stephenplusplus stephenplusplus removed status: blocked Resolving the issue is dependent on other work. don't merge labels Mar 15, 2016
@stephenplusplus
Copy link
Contributor Author

I'm subscribed to that one, so I'll be sure to follow along and make the updates here when the PR is merged and a new release is cut. For now, @callmehiphop PTAL!

@@ -244,9 +265,9 @@ GrpcService.prototype.request = function(protoOpts, reqOpts, callback) {
* @param {*} data - An object or array to iterate over.
* @return {*} - The converted object.
*/
GrpcService.prototype.convertBuffers_ = function(data) {
GrpcService.convertBuffers_ = function(data) {

This comment was marked as spam.

This comment was marked as spam.

@callmehiphop
Copy link
Contributor

@stephenplusplus aside from the convertBuffers_ bug, everything LGTM!

callmehiphop added a commit that referenced this pull request Mar 21, 2016
@callmehiphop callmehiphop merged commit 3965972 into googleapis:master Mar 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants