Skip to content

Conversation

@zpencer
Copy link
Contributor

@zpencer zpencer commented Sep 11, 2018

  • Log using new proto definition, this is effectively a rewrite
  • Remove io.grpc.BinaryLog.CallId because a call ID is now an AtomicLong
  • Add the concept of "always included" and "never included" metadata
    keys. This is needed because grpc-status-details-bin is already
    logged in the binlog msg, and we will log grpc-trace-bin for the
    census info.

@zpencer zpencer changed the title V1 binlog core,services: v1 binlog Sep 11, 2018
@zpencer zpencer requested a review from ejona86 September 20, 2018 21:42
- Log using new proto definition
- Remove io.grpc.BinaryLog.CallId because a call ID is now an AtomicLong
- Add the concept of "always included" and "never included" metadata
  keys. This is needed because grpc-status-details-bin is already
  logged in the binlog msg, and we will log grpc-trace-bin for the
  census info.
- unit tests are effectively rewritten
*/
class BinaryLogProviderImpl extends BinaryLogProvider {
// avoid using 0 because proto3 long fields default to 0 when unset
private static final AtomicLong counter = new AtomicLong(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this become static?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an effort to simplify the call id for the grfc, I made it so that the call id is a unique uint64. I didn't want the same ID to appear in different files if multiple log files are used. https://github.com/grpc/proposal/blame/master/A16-binary-logging.md#L107


TimeProvider SYSTEM_TIME_PROVIDER = new TimeProvider() {
final long offsetNanos =
TimeUnit.MILLISECONDS.toNanos(System.currentTimeMillis()) - System.nanoTime();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the clock changes, then the timestamps will be incorrect. Is that what is supposed to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's simply report the current system time. There's no requirement to have a high resolution timestamp.

@zpencer zpencer merged commit da87ffb into grpc:master Sep 27, 2018
@zpencer zpencer deleted the v1-binlog branch September 27, 2018 20:19
@lock lock bot locked as resolved and limited conversation to collaborators Dec 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants