search

cloudendpoints / esp

Extensible Service Proxy
https://cloud.google.com/endpoints/
BSD 2-Clause "Simplified" License
281 stars 73 forks source link

Unable to specify JSON message print options #227

Closed rkdirect closed 5 years ago

rkdirect commented 7 years ago

The following excerpt defines the print options available in grpc/protobuf:

struct JsonPrintOptions {
  bool add_whitespace;
  bool always_print_primitive_fields;
  bool always_print_enums_as_ints;
  bool preserve_proto_field_names;
}

Unfortunately some of these values are simply never set, and other are hard coded from the "JsonOptions" bitmask being passed into the function and hard-coded elsewhere. (see https://github.com/cloudendpoints/esp/blob/master/src/api_manager/utils/marshalling.cc#L53)

IDEALLY:

What we would ideally like is a means of decorating the message type in the proto to provide these json transformation options:

boolean always_print_primitive_fields = false;
boolean always_print_enums_as_ints = false;
boolean preserve_proto_field_names = false;

This would allow esp or even the underlying protobuf/grpc layer to inspect the descriptor, determine the contract formatting type, and provide the appropriate output. This is a fairly trivial change either in esp's "src/api_manager/utils/marshalling.cc" or "src/google/protobuf/util/json_util.cc" in google/protobuf; however, I also realize the cost associated with a release is non-zero.

Background/Use Case:

We have an existing api we are trying to replace originally written in nodejs with open api and express. We are reimplementing this service on a new backend storage and we were attempting to use GRPC and ESP. The service currently expresses an API where both empty string "", and zero 0 values are expected by the client code. In addition the service also currently exposes fields with an underscore "_" in them.

Without being about to ensure that the fields are both written to the output, and left in their original name we are kinda lost how to proceed without trying to simultaneously retool all client uses. Although untested, I believe specifying [jsonname] in the proto may solve for the "" issue, it still leaves us shy of the mark.

lizan commented 7 years ago

@rkdirect thanks for bringing this up. The gRPC<->JSON translating is using grpc-httpjson-transcoding library, not the marshalling.cc. A PR is already merged in the library https://github.com/grpc-ecosystem/grpc-httpjson-transcoding/pull/13 . I need to plumb them up, and add the flags to service config to support configuring it.

rkdirect commented 7 years ago

@lizan I see, missed that part of the puzzle ;)

You mention:

I need to plumb them up, and add the flags to service config to support configuring it.

I'm using the NodeJS server, will this be available to me as an api/option, or proto extension?

Honestly I've had a lot of trouble with the nodejs server options, I never did get either 'grpc.max_send_message_length' or 'grpc.max_receive_message_length' to work. So I was hopeful this could be specified on a message level within the proto (instead of the entire service)? It would seems like an appropriate extension as, at least for my case, this is really part of the contract and is only important in a single sub-message. Thoughts?

lizan commented 7 years ago

Honestly I've had a lot of trouble with the nodejs server options, I never did get either 'grpc.max_send_message_length' or 'grpc.max_receive_message_length' to work. That sounds an gRPC issue, you might want to file an issue with gRPC for more details. ESP set this to MAX_INT so you won't be capped by ESP at least.

To have ESP support this, it is irrelevant to what server you use behind ESP, the only thing effect is the protobuf descriptor and service config you provide to ESP. The traffic between ESP and your backend server is gRPC with binary protobuf, it doesn't contain any of the JSON conversion information.

So I was hopeful this could be specified on a message level within the proto (instead of the entire service)? It would seems like an appropriate extension as, at least for my case, this is really part of the contract and is only important in a single sub-message. Thoughts?

If you want a message-level control for this, it would be better to file an issue with protobuf, since ESP don't have control what "src/google/protobuf/util/json_util.cc" in google/protobuf does for each sub-messages. What ESP can support is either service-level control, or method-level control.

rkdirect commented 7 years ago

Cool enough, and thanks for the explanation.

I'm not sure I agree with the claim that ESP is unlimited... I submitted this to grpc as an error here: grpc/issues/12012 and the response I got was the grpc service was sending more than 4mb when configured; however, ESP returns the following error: "message": "Received message larger than max (6132324 vs. 4194304)", Which tells me that the message failed to receive, not send. Does this have something to do with the transformation to JSON in ESP enforcing this limit?

Anyway back to the actual issue, specifying always_print_primitive_fields etc...

Do you know if this will be controlled by the proto or is this something else? What would that look like?

lizan commented 7 years ago

What version are you using? ESP won't cap that since 1.2.0, ESP use same gRPC client for both pass-through and transcoding.

Yes, I meant the always_print_primitive_fields is not for message level, but we can implement in either service level or method level.

For example you have proto like this:

message SubMessage {
  int32 bar = 1;
}
message ParentMessage {
  SubMessage sub = 1;
  int32 foo = 2;
}

service SampleService {
  rpc sampleMethod(google.protobuf.Empty) returns (ParentMessage) {
    option (google.api.http).get = "/v1/sample";
  }
}

We can provide an option for sampleMethod or SampleService, which means you will get both foo and bar appears in JSON, or neither of them do. If you want primitive field in SubMessage appears in JSON but not foo, then we need to change protobuf and out of scope here.

I'm working on how it would look like, for method level, it will likely be an another flag in google.api.http.

rkdirect commented 7 years ago

@lizan Well of course I was just being an id-10T, my development environment was pulling endpoints:1.0, instead of endpoints:1 (latest). So mystery solved and working in production :+1:

Looking forward to being able to resolve this, I assume it might look something like this?:

service SampleService {
  rpc sampleMethod(google.protobuf.Empty) returns (ParentMessage) {
    option (google.api.http) {
      get = "/v1/sample";
      always_print_primitive_fields = true;
      always_print_enums_as_ints = true;
      preserve_proto_field_names = true;
  }
}
lizan commented 7 years ago

Glad to hear that :)

Yes that is my thought, we will figure it out

rkdirect commented 6 years ago

Have we made any progress on this? This is causing a lot of confusion with developers and slowing adoption within our organization.

rkdirect commented 6 years ago

@lizan Can you please provide an update on when this might be available (even a preview/beta)?

lizan commented 6 years ago

@rkdirect Sorry for the delay, we had discussion internally for how to support this in google.api and working on updating those protos. Once the protos is updated, the work in ESP side is not large.

jeremylorino commented 6 years ago

@lizan any update on this? We have been digging through the code trying to see if we could fork this and work around it but have been unsuccessful.

Any suggestions?

lizan commented 6 years ago

@jeremylorino Sorry for the delay, the internal discussion around the service config google.api is still ongoing and I don't have ETA for that yet.

However, there is a workaround, if ESP instance level configuration works for you, which means that you can specify JSON print options per ESP instance, not per service nor per method.

Here is the implementation idea:

Then you should be able to specify the server config via command line options of start_esp.

If this sounds like a workaround for you, we're also open to PR for this and I'm happy to take the review.

jeremylorino commented 6 years ago

Awesome! I actually cloned the repo after my comment and was proceeding to change the transcoding factory default print options to true rather than false. And use this docker image internally for us as the interim solution.

But I am much more inclined to implement the "real" solution you outlined.

I'll let you know when it is ready. On Tue, Oct 17, 2017 at 7:51 PM Lizan Zhou notifications@github.com wrote:

@jeremylorino https://github.com/jeremylorino Sorry for the delay, the internal discussion around the service config google.api is still ongoing and I don't have ETA for that yet.

However, there is a workaround, if ESP instance level configuration works for you, which means that you can specify JSON print options per ESP instance, not per service nor per method.

Here is the implementation idea:

Then you should be able to specify the server config via command line options of start_esp.

If this sounds like a workaround for you, we're also open to PR for this and I'm happy to take the review.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cloudendpoints/esp/issues/227#issuecomment-337426297, or mute the thread https://github.com/notifications/unsubscribe-auth/AEyA3vx4yzm-MFZT2-dNjY6F5WkUWOmDks5stUuZgaJpZM4OpFtz .

--

  • google is watching
rkdirect commented 6 years ago

@lizan The solution you recommended has been sent via PR https://github.com/cloudendpoints/esp/pull/294

As noted in the PR, we are currently unable to verify the change as we have a few unknowns:

  1. We do not have a means to produce a docker image, the documentation does not mention how to build this and it appears it requires a ".deb" package. I assume we are missing one or more build steps.
  2. Does building the package and docker image require a specific operating system and/or version?
  3. We are not aware of how to specify the "server config" or these "experimental" options when starting the service through the start_esp script.
rkdirect commented 6 years ago

@lizan If I understand your response correctly, there is not a way to specify this option directly from the command line. To add that ability I would need to:

First, parse and define the argument in start_esp.py

# always_print_primitive_fields.
parser.add_argument('--print_primitive',
    default='false',
    help=argparse.SUPPRESS)

Then I will need to add the template argument here: https://github.com/cloudendpoints/endpoints-tools/blob/master/start_esp/start_esp.py#L147

conf = template.render(
         service_configs=args.service_configs,
         management=args.management,
         rollout_id=args.rollout_id,
         rollout_strategy=args.rollout_strategy,
        print_primitive=args.print_primitive)

... and consume that argument in the default template here: https://github.com/cloudendpoints/endpoints-tools/blob/master/start_esp/server-auto.conf.template#L33

experimental {
  disable_log_status: true
  always_print_primitive_fields: ${print_primitive}
}

Does that sound reasonable, and is this something we could PR/merge in?

lizan commented 6 years ago

@rkdirect sounds good to me. lets make the flag name more verbose, transcoding_ always_print_primitive_fields or transcoding_always_print_primitive, to make it clear this only affect transcoding.

Thanks a lot for the effort.