Tracking issue for BinaryLog public API

perezd commented 5 years ago

Is there a design document for how this might be used in practice?

carl-mastrangelo commented 5 years ago

This issue is for the feature being stable, not so much usage. I don't believe there are any tools that use the binary log format currently.

perezd commented 5 years ago

OK, I wasn't able to even really easily find what the format of the binary log is anywhere. Should this not be explored by even the brave? :)

ejona86 commented 4 years ago

~~Requirement before going stable: update to v1/binarylog.proto from v1alpha/binarylog.proto (#6694)~~ I was wrong. It is already on v1/binarylog.proto

sergiitk commented 12 months ago

Note for the API stab meeting: would it make sense to make it an interface?

Current:

public abstract class BinaryLog implements Closeable {

  public abstract <ReqT, RespT> ServerMethodDefinition<?, ?> wrapMethodDefinition(
      ServerMethodDefinition<ReqT, RespT> oMethodDef);

  public abstract Channel wrapChannel(Channel channel);
}

Interface:

public interface BinaryLog extends Closeable {

  <ReqT, RespT> ServerMethodDefinition<?, ?> wrapMethodDefinition(
      ServerMethodDefinition<ReqT, RespT> oMethodDef);

  Channel wrapChannel(Channel channel);
}

sergiitk commented 8 months ago

The list of files with @ExperimentalApi associated with this ticket:

❯ git grep -lF '@ExperimentalApi("https://github.com/grpc/grpc-java/issues/4017")'
api/src/main/java/io/grpc/BinaryLog.java
api/src/main/java/io/grpc/ManagedChannelBuilder.java
api/src/main/java/io/grpc/ServerBuilder.java
services/src/main/java/io/grpc/protobuf/services/BinaryLogSink.java
services/src/main/java/io/grpc/protobuf/services/BinaryLogs.java
services/src/main/java/io/grpc/services/BinaryLogs.java

larry-safran commented 8 months ago

To absorb changing BinaryLog to an interface we would want to use a 'stepping stone' process where we would have both the old and new versions available together so we would have to keep BinaryLog as an abstract class and define a new interface with a different name and make the change over 3 releases. So don't change to an interceptor.

Ideally would move it out of Channel to be an interceptor

Difficult because of OpenCensus - OpenTelemetry doesn’t have the issue
Could keep the method, but emulate it as an interceptor during ManagedChannelBuilder

BinaryLogs - delete deprecated method

BinaryLogSink

inconsistency in throws IoException not on write but is on close (and constructor has) it. Probably don't want it on write since we wouldn't want to fail the RPC just because can't log it and even worse could in the future be async and not know about the failure when return. However, might make sense for close so okay the way it is.
Having MessageLite works fine for temp files, but if sending over the wire might want to use JSON which can't be done with MessageLite.

Shouldn't call close on the sync when config fails to parse in BinaryLogs.

2 BinaryLogs classes, the one in services could be deleted.

Multiple opinions - deferring decision on stabilization.

rob-pomelo commented 4 months ago

hi, i think the pr attached incorrectly deleted the bazel build target for the non deprecated proto target https://github.com/grpc/grpc-java/pull/10832, everything but line 75 should have been left https://github.com/grpc/grpc-java/pull/10832/files#diff-c9799c0c8192c49ded7f512311aa20eccf5d61e68b0e2564c372d63cac06531cL75

ejona86 commented 4 months ago

@sergiitk, see @rob-pomelo's comment. It does seem we need to restore the target and only delete the one line. (I had thought we had a separate target for the copy of the API under protobuf/services, but, no, it seems we had one target.)

sergiitk commented 3 months ago

Sorry about that. Restoring the target with #11292. @ejona86 / @rob-pomelo - let me know if you want me to backport the PR once it's merged.

grpc / grpc-java

Tracking issue for BinaryLog public API #4017