Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Span id encoding#719

Merged
songy23 merged 14 commits intocensus-instrumentation:masterfrom
mhindery:span_id_encoding
Jul 17, 2019
Merged

Span id encoding#719
songy23 merged 14 commits intocensus-instrumentation:masterfrom
mhindery:span_id_encoding

Conversation

@mhindery
Copy link
Copy Markdown
Contributor

@mhindery mhindery commented Jul 16, 2019

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.

@mhindery mhindery requested review from a team, c24t, reyang and songy23 as code owners July 16, 2019 17:20
@mhindery mhindery mentioned this pull request Jul 16, 2019
Closed
reyang
reyang reviewed Jul 16, 2019
View reviewed changes
reyang
reyang reviewed Jul 16, 2019
View reviewed changes
opencensus/trace/propagation/google_cloud_format.py
header = '{}/{};o={}'.format(
trace_id,
span_id,
int(span_id, 16),
Copy link
Copy Markdown
Contributor

@reyang reyang Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

@mhindery mhindery Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@reyang reyang Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@reyang reyang Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
reyang approved these changes Jul 16, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

songy23
songy23 approved these changes Jul 16, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown

googlebot commented Jul 17, 2019

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
Copy link
Copy Markdown

googlebot commented Jul 17, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

reyang
reyang reviewed Jul 17, 2019
View reviewed changes
@mhindery
Copy link
Copy Markdown
Contributor Author

mhindery commented Jul 17, 2019

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
Copy link
Copy Markdown
Contributor

reyang commented Jul 17, 2019

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
Copy link
Copy Markdown
Contributor Author

mhindery commented Jul 17, 2019

Added some extra tests, now all checks pass.

@songy23 songy23 merged commit a86e5c8 into census-instrumentation:master Jul 17, 2019
@mhindery mhindery deleted the span_id_encoding branch July 17, 2019 18:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@c24t c24t Awaiting requested review from c24t

2 more reviewers

@reyang reyang reyang approved these changes

@songy23 songy23 songy23 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mhindery @googlebot @reyang @songy23