Span id encoding

[mhindery]

The span ID is currently a hex-encoded 16-char string, which shouldn't be the case. It should be a decimal, see census-ecosystem/opencensus-go-exporter-stackdriver#169 (comment)

@mhindery according this doc spanId should be decimal.
For first span it should be 0. example copied from the doc

curl "http://www.example.com" --header "X-Cloud-Trace-Context:
  105445aa7843bc8bf206b120001000/0;o=1"

This breaks compatibility between languages as (e.g. in my case) python and go don't propagate traces correctly.

I changed this by the modifying the check_span_id and generate_span_id in the SpanContext class to not generate a hexadecimal representation, but just a decimal one. If that would be a problem for other parts of the code which require hex representation, it could be kept, and converted in the to_header() method of the GoogleCloudFormatPropagator, however, if it isn't necessary to actually have a hex representation then the conversion can be omitted altogether as it's simpler to just have it as decimal everywhere.

[reyang]

We probably need to update from_header as well.

@c24t FYI - this is a potential breaking change (e.g. if customers mix use old version and the latest version of OpenCensus Python SDK).

[mhindery]

the from_header has been updated, it contains

            if span_id:
                span_id = '{:016x}'.format(int(span_id))

to do the conversion, or do you mean something additional?

[reyang]

@mhindery please update CHANGELOG.md if this is a breaking change. Thanks.

[reyang]

Adding @c24t to comment from Google side.

I cannot find the official spec for X-Cloud-Trace-Context, and I'm a bit worried that this change might break existing stuff for Google (considering there are system tests running as part of the CI, it is a bit surprising to me that we've been sending the wrong format to Stackdriver).

[reyang]

The proposed change looks good to me.
We would need @c24t @songy23 to comment from Google's perspective.

[songy23]

It makes sense to me to only update the encoding in google cloud format (AFAIK it's only used in AppEngine Standard). FYI in Java SpanID is also encoded as a number: https://github.com/census-instrumentation/opencensus-java/blob/52f38e48e2ac6cb65e28dcd97b4f7e9650357bba/contrib/appengine_standard_util/src/main/java/io/opencensus/contrib/appengine/standard/util/AppEngineCloudTraceContextUtils.java#L93.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. 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.

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[mhindery]

I don't get why the coverage claims the lines 65-68 of opencensus/trace/propagation/google_cloud_format.py are not covered. There is a system test TestBasicTrace which calls the from_header method on the propagator with a valid header, and those lines it claims are missing, are handled in that method.

[reyang]

I don't get why the coverage claims the lines 65-68 of opencensus/trace/propagation/google_cloud_format.py are not covered. There is a system test TestBasicTrace which calls the from_header method on the propagator with a valid header, and those lines it claims are missing, are handled in that method.

I guess the coverage is complaining about a missing branch coverage:

L65         if span_id:
L66             span_id = '{:016x}'.format(int(span_id))
L67
L68         span_context = SpanContext(

What we need is two test cases to cover the branch statement:

1. if span_id evaluates to True.
2. if span_id evaluates to False.

[mhindery]

Added some extra tests, now all checks pass.