Implement LoggingAsync for GRPC Worker Services

[xldrx]

This allow RecvTensor events to show up in StepStats and in turn in Chrome Tracing format.

[tensorflow-jenkins]

Can one of the admins verify this patch?

[xldrx]

@pbar @mrry @suharshs Could you please take a look at this?

[mrry]

Reassigning this to @prb12, since he knows the ins and outs of the logging path better than I do. A few quick observations though:

• Instead of modifying the BaseRendezvousMgr and related classes to capture this information, please use the existing stubs in GrpcWorkerCache:

  tensorflow/tensorflow/core/distributed_runtime/rpc/grpc_worker_cache.cc

  Lines 86 to 92 in c0662f1

  	void SetLogging(bool v) override { logger_.SetLogging(v); }
  	void ClearLogs() override { logger_.ClearLogs(); }
  	bool RetrieveLogs(int64 step_id, StepStats* ss) override {
  	return logger_.RetrieveLogs(step_id, ss);
  	}

• The changes in master_session.cc are potentially controversial, because they could cause a lot of additional logging to be generated when FULL_TRACE is specified. We might need an additional TraceLevel for this case.

• Run clang-format -style=Google over the code to take care of style issues (which should trigger a failure in the presubmit).

[xldrx]

Thanks @mrry for the feed back:

• I changed the implementation to use SessionMgr instead of *RendezvousMgr.
  Note that unlike other worker rpcs, LoggingRequest in LoggingAsync rpc does not have any session handle, therefore all sessions should be touched. Both implementations (previous one, and this one) iterates over all sessions (previously through RendezvousMgr now SessionMgr), then uses GrpcWorkerCache calls in each session.

• @pbar: Thoughts on changes in master_session.cc?

[sb2nov]

@xldrx can you look into the merge conflicts.

[xldrx]

Hey @sb2nov,
It seems that Paul is swamped. Could you review this PR?

[sb2nov]

@xldrx I don't know enough to review this right now. @prb12 ping.

[prb12]

If both clear and fetch_step_id are used the semantics are unclear, but I would have thought it made more sense to retrieve any previously collected data before clearing?

[xldrx]

Great observation. Fixed.

[prb12]

Thanks for addiing this!

With regard to an additional TRACE_LEVEL - I don't think that this is likely to be a problem.

I'm assuming this doesn't have a noticeable performance impact when tracing is not enabled?

One minor question about the case where clear and retrieve are done at the same time.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[googlebot]

CLAs look good, thanks!

[xldrx]

@prb12 Thanks for the review.

I'm assuming this doesn't have a noticeable performance impact when tracing is not enabled?

This PR does nothing when tracing is not enabled.

One minor question about the case where clear and retrieve are done at the same time.

Great observation. Fixed.

[rmlarsen]

@xldrx This appears to have numerous broken tests. Can you please take a look?

[rmlarsen]

@xldrx Thanks for the contribution!