chore: Add 'grpc.client.deadline' tag to track gRPC client's deadline value

[nvh0412]

What does this PR do?
resolve #2199

Motivation
I'm using Datadog tracing to methodically trace data from the whole of our internal system (Web server, gRPC, ...). As I use multi-threaded servers to run our gRPC server, hence I'm facing many DeadlineExceed exceptions from the gRPC client ruby because the thread took so long to finish its job, and the timeout setting isn't enough.

Despite talking about the solution, I want to know the value of client timeout when it sends the request to the gRPC server. In this way, I would know how many redundant thread executions are while its client call is terminated because of timeout.

So I'm adding grpc.client.deadline tag to track gRPC client's deadline value

[TonyCTHsu]

Hi @nvh0412 , Thanks for the contribution!

Could you provide a positive example from spec which verify the deadline has been set correctly with a value (ActiveCall#deadline would return a Time object).

Also please rebase the branch to the latest master to avoid failing spec in old Rubies. 😄

[nvh0412]

Thank you @TonyCTHsu , addressed.

[TonyCTHsu]

I was wondering whether information is accurate enough to diagnose the DeadlineExceed exception.

What would a good general default value for grpc client deadline?

I am asking because when Time object serialized by to_s, it is returning 2022-08-03 17:51:59 +0200.
This is only providing the resolution in seconds. I assumed that the resolution to milliseconds would be more useful?

[nvh0412]

Fair enough, the Time object's serialised value might also affect the Datadog query in our Dashboard. Milliseconds would be better, thanks for suggesting me @TonyCTHsu, addressing.

[nvh0412]

Please help me review this PR again, thank you.

[codecov-commenter]

Codecov Report

Merging #2200 (60d0e4f) into master (8c9806d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2200   +/-   ##
=======================================
  Coverage   97.48%   97.48%           
=======================================
  Files        1043     1043           
  Lines       54429    54438    +9     
=======================================
+ Hits        53058    53068   +10     
+ Misses       1371     1370    -1     

Impacted Files	Coverage Δ
...tracing/contrib/grpc/datadog_interceptor/client.rb	97.72% <100.00%> (+0.29%)	⬆️
lib/datadog/tracing/contrib/grpc/ext.rb	100.00% <100.00%> (ø)
...ng/contrib/grpc/datadog_interceptor/client_spec.rb	100.00% <100.00%> (ø)
...adog/tracing/contrib/sidekiq/client_tracer_spec.rb	100.00% <0.00%> (+2.08%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[marcotc]

👋 @nvh0412, could you move the time conversion logic of deadline (from object to number) to the find_deadline method?

I think it looks cleaner if the caller here only has to set the deadline, like so:

Suggested change

	span.set_tag(Ext::TAG_CLIENT_DEADLINE, (deadline.to_time.to_f * 1000).to_i) if deadline
	span.set_tag(Ext::TAG_CLIENT_DEADLINE, deadline) if deadline

[nvh0412]

Hi @TonyCTHsu @marcotc I addressed your feedbacks, also squashed all commit to one, that's why I have to do a push force. Please help me review this PR again, thank you

[TonyCTHsu]

👋 @nvh0412
According grpc. There is no true answer for a good timeout. But it seems to be legit that millisecond resolution would be helpful.

There is no single answer to “What is a good deadline/timeout value?”. Your service might be as simple as the Greeter in our quick start guides, in which case 100 ms would be fine. Your service might be as complex as a globally-distributed and strongly consistent database. The deadline for a client query will be different from how long they should wait for you to drop their table.

I took a deeper dive into grpc , and yes. The deadline value could be a Time or TimeSpec object. When user provides a positive number, it would ends up to become a Time object. It is also possible providing TimeSpec and nil which return TimeSpec.

When I tried to write test examples for TimeSpec, I realized that there are three TimeSpec object, referenced by TimeConsts::ZERO , TimeConsts::INFINITE_FUTURE and TimeConsts::INFINITE_PAST. All three of them are referring to value back in 1960s. I wonder whether those TimeSpec values are important/useful to your case?

For Time serialization,

irb(main):001:0> require 'time'
=> true
irb(main):002:0> Time.now.utc.iso8601(3)
=> "2022-08-04T17:24:09.271Z"

This this would provide a human readable and interchangable format, which string comparison should also work for dashboard query.

if TimeSpec is not important for visibility. Perhaps we don't need to tag it. But if it is, consider placing a tag grpc.client.deadline.typewith [TIMESTAMP, INFINITE_FUTURE, INFINITE_PAST, ZERO] and grpc.client.deadline.timestamp with serialized time. This would avoid confusion of ill-formed data of those.

[nvh0412]

I don't think TimeConsts::ZERO, TimeConsts:: INFINITE_FUTURE, TimeConsts::INFINITE_PAST are valuable for us. Also, I just figured out that if we tag this deadline on the client interceptor, its value is always Time ruby, TimeSpec only appear on Server interceptor.

So I do believe we can come up with that we don't need to handle TimeSpec type in this PR because we only change the client interceptor. But I will address the code that ensures we won't tag the deadline if it's TimeSpec.

As for the serialized value of Time object, it's a fifty-fifty split milisecond number or a string value (iso8601 format). Because the millisecond value would help others who want to calculate/compare with other values easier than a string value, I believe. (i.e duration value), so let's keep it as millisecond for now. Wdyt? @TonyCTHsu

[TonyCTHsu]

If TimeSpec is not interested, let's just skip it.

I believe I could approve the change after iso8601 format serialization 😄 .

[nvh0412]

Addressed @TonyCTHsu , there are also some flaky tests on the master branch https://app.circleci.com/pipelines/github/DataDog/dd-trace-rb?branch=master.

[TonyCTHsu]

@nvh0412 Thanks for the great work! 😘