census-ecosystem / opencensus-go-exporter-stackdriver

OpenCensus Go exporter for Stackdriver Monitoring and Trace
Apache License 2.0
67 stars 79 forks source link

update SpanID type in http propagation to adhere to trace API V2 spec #254

Open anovitskiy opened 4 years ago

anovitskiy commented 4 years ago

According to https://cloud.google.com/trace/docs/reference/v2/rest/v2/projects.traces/batchWrite

[SPAN_ID] is a unique identifier for a span within a trace; it is a 16-character hexadecimal encoding of an 8-byte array.

codecov-io commented 4 years ago

Codecov Report

Merging #254 into master will decrease coverage by 0.01%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   72.02%   72.01%   -0.02%     
==========================================
  Files          17       17              
  Lines        1691     1690       -1     
==========================================
- Hits         1218     1217       -1     
  Misses        397      397              
  Partials       76       76              
Impacted Files Coverage Δ
propagation/http.go 60.00% <100.00%> (-1.30%) :arrow_down:

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 e191b7c...5ff15ed. Read the comment docs.

anovitskiy commented 4 years ago

@rghetia @bogdandrutu could one of you gents please take a look at this when you get a chance?

rghetia commented 4 years ago

@rghetia @bogdandrutu could one of you gents please take a look at this when you get a chance?

@anovitskiy thanks for the changes. Changes look good however I have one question. Would there be a case where a user would need to use v1 format?

anovitskiy commented 4 years ago

@rghetia @bogdandrutu could one of you gents please take a look at this when you get a chance?

@anovitskiy thanks for the changes. Changes look good however I have one question. Would there be a case where a user would need to use v1 format?

Thanks, @rghetia Luckily, this is completely backwards compatible since an 8-byte unsigned integer can be encoded into hexadecimal and decoded back to the same value.

rghetia commented 4 years ago

@rghetia @bogdandrutu could one of you gents please take a look at this when you get a chance?

@anovitskiy thanks for the changes. Changes look good however I have one question. Would there be a case where a user would need to use v1 format?

Thanks, @rghetia Luckily, this is completely backwards compatible since an 8-byte unsigned integer can be encoded into hexadecimal and decoded back to the same value.

What I meant was that the new format is encoding spanid into a hex string but if on the other side if it is expecting decimal string then it won't work.

anovitskiy commented 4 years ago

@rghetia @bogdandrutu could one of you gents please take a look at this when you get a chance?

@anovitskiy thanks for the changes. Changes look good however I have one question. Would there be a case where a user would need to use v1 format?

Thanks, @rghetia Luckily, this is completely backwards compatible since an 8-byte unsigned integer can be encoded into hexadecimal and decoded back to the same value.

What I meant was that the new format is encoding spanid into a hex string but if on the other side if it is expecting decimal string then it won't work.

Hopefully for most users the calls to SpanContextToRequest() and SpanContextFromRequest() are in the same library but the release version should indicate that this is a potentially breaking change. Open to ideas on how else to version this change.

rghetia commented 4 years ago

Hopefully for most users the calls to SpanContextToRequest() and SpanContextFromRequest() are in the same library but the release version should indicate that this is a potentially breaking change. Open to ideas on how else to version this change.

Breaking change is not desirable at this point given that opencensus is replaced by opentelemetry. One option is to

punya commented 3 years ago

A third option would be to preserve the original trace and span IDs as strings instead of decoding and encoding. Was the original design chosen in order to conserve memory?