census-instrumentation / opencensus-erlang

A stats collection and distributed tracing framework
https://opencensus.io
Apache License 2.0
134 stars 39 forks source link

oc_trace:start_span/2,3 accepts undefined SpanCtx #154

Closed essen closed 5 years ago

essen commented 5 years ago

It calls the internal newspan function which accepts undefined.

This is to make Dialyzer pass when calling the function directly to create a new trace that is independent from the one managed by ocp.

googlebot commented 5 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

tsloughter commented 5 years ago

Thanks! I think we should also consider making the span_ctx type it self #span_ctx{} | undefined because undefined is used to act as a noop trace and it would probably be best to force that all functions taking a spanctx support the noop trace. But this is good for now.

essen commented 5 years ago

I wasn't sure about that but that's good to know. I can probably do a better patch next week if you want.

deadtrickster commented 5 years ago

@essen if this not blocking, could wait I guess.

essen commented 5 years ago

It's just Dialyzer complaining. Me I can wait, up to you if you want to merge earlier.

tsloughter commented 5 years ago

I was just going to merge this, don't know that switching span_ctx itself to also be possibly undefined will be a quick change so may as well get this in.

But @essen has to accept the CLA first :).

essen commented 5 years ago

I signed it!

googlebot commented 5 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.