Refactor tracing implements

[XSAM]

Split from #275

• Add tracing interceptor.
• Add opentracing provider.
• Add opentelemetry provider.

[codecov-commenter]

Codecov Report

Merging #312 into v2 will decrease coverage by 6.54%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##               v2     #312      +/-   ##
==========================================
- Coverage   83.58%   77.03%   -6.55%     
==========================================
  Files          30       30              
  Lines         932      932              
==========================================
- Hits          779      718      -61     
- Misses        114      168      +54     
- Partials       39       46       +7     

Impacted Files	Coverage Δ
chain.go	0.00% <ø> (-90.91%)	⬇️
interceptors/tags/fieldextractor.go	84.84% <ø> (-0.45%)	⬇️
interceptors/logging/payload.go	83.07% <50.00%> (+0.46%)	⬆️
interceptors/retry/retry.go	67.74% <72.72%> (-8.51%)	⬇️
wrappers.go	66.66% <0.00%> (-33.34%)	⬇️
interceptors/retry/options.go	73.17% <0.00%> (-7.32%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 880c174...0c87891. Read the comment docs.

[XSAM]

@bwplotka I need some review to confirm the tracing interface since it implements parts of interceptors.Reporter. 😁

[bwplotka]

Hey, can we mention what is high lvl plan here? If I understand correctly it is required to have both opentelemetry and opentracing, right?

TBH in this case I would first add opentelemetry package and check if have anything to reuse between those two or not.

Problem with this is that opentracing and opentelemetry are actually interfaces. So creating an interface that fits both might be an overkill. What do you think? (:

[XSAM]

Well, opentracing and OTEL (opentelemtry) do have a familiar interface of span to record events.

opentracing: LogFields, OTEL: AddEvent.

But the problem is the behavior from two libs are not the same while handling PostMsgSend and PostMsgReceive.

For instance, when invoking PostMsgSend, opentracing will do nothing with LogFields since it doesn't require to record gRPC message info, but OTEL will record that info via AddEvent.

Furthermore, attributes which been using in the event are different.

OTEL:

span.AddEvent(ctx, "message",
	kv.Key("message.type").String("SENT"),
	kv.Key("message.id").Int(id),
	kv.Key("message.uncompressed_size").Int(proto.Size(body)),
)

So, I think there are two solutions to this.

1. Add AddEvent function for span interface. But it will force opentracing to add the same key and attributes to events like OTEL, or opentracing have to do some weird mapping to drop these non-standard events.

2. Add PostMsgSend and PostMsgReceive for span interface. Weird but more flexible.

---

• https://github.com/opentracing/specification/blob/master/semantic_conventions.md
• https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/rpc.md#events

[bwplotka]

Hi @XSAM Let's get back to this!

I feel like option number 2 is too similar too reporter, so kind of no point in having separate span interface, no? (:

That's why we have two options I think

1. as your 1, we could try that.

2. Actually have no common interface among those, just have 2 different providers/opentelemetryandproviders/opentracing` ^^

[XSAM]

You're right, option 2 is too similar with interceptors.Reporter.

Now, I use option 1 as a solution.

[yashrsharma44]

Seems like this PR would address #277 . @XSAM can you let us know if you are interested in working on rebasing this PR and meeting the acceptance criteria? Would be happy to take over this PR, otherwise 😄

Related Issue - #277

[XSAM]

@yashrsharma44 I will continue to work on this PR. Thanks for the reminder. 😄

[XSAM]

I updated this PR. @yashrsharma44 @bwplotka PTAL ;)

This PR seems huge and not easy to review. I can split this if you prefer.

[XSAM]

Any updates on this one?

[yashrsharma44]

Hi @XSAM!
Extremely sorry for the late review, I was busy with my internship requirements, so wasn't able to take a look at the PR.

• Can you split the changes for -
  • Add tracing interceptor.
  • Add opentracing provider.
  • Add opentelemetry provider.

in a separate PR? Would be easy to review the changes.

• Also, since we are using multi-modules, the test would not pass in GitHub Actions/Circle CI/Travis, you would certainly need to break this PR into interceptors and providers. Here are a couple of PRs which were merged in a similar manner - (1) improve v2 rate-limiter #380 (2) [Rate-limit provider]: Add token bucket implementation of rate-limiter #386

• Also can you rebase your PR with the latest changes?

I would be happy to review the changes and help you out in finishing up a subpart, in case you don't have time to address all 😄

[bwplotka]

I think it's actually reviewable, in one place as those are new things mostly. But it will take time.

Still, I think we should do it before v2?

[bwplotka]

Are you around @XSAM to work on it (rebase etc)?

Also I am removing tracing here, to avoid modifications there that will have to be there as part of this Pr anyway (#394 (comment))

[XSAM]

Are you around @XSAM to work on it (rebase etc)?

Also I am removing tracing here, to avoid modifications there that will have to be there as part of this Pr anyway (#394 (comment))

Sorry for the late response, I was busy recently.

I'm still working on it; will try to split these changes for clarity.