opencensus-go-exporter-stackdriver icon indicating copy to clipboard operation
opencensus-go-exporter-stackdriver copied to clipboard

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

Open anovitskiy opened this issue 4 years ago • 8 comments

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.

anovitskiy avatar Mar 25 '20 21:03 anovitskiy

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.

codecov-io avatar Apr 02 '20 17:04 codecov-io

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

anovitskiy avatar Apr 02 '20 17:04 anovitskiy

@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?

rghetia avatar Apr 02 '20 21:04 rghetia

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

anovitskiy avatar Apr 02 '20 22:04 anovitskiy

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

rghetia avatar Apr 08 '20 21:04 rghetia

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

anovitskiy avatar Apr 08 '20 21:04 anovitskiy

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

  • Add version field in HTTPFormat and a new func to create version 2 object. Use conditional encoding/decoding based on the version.
type HTTPFormat struct{
	version int
}
const (
	httpFormatVersion2 = 2
)
func NewHTTPFormatV2() HTTPFormat {
	return HTTPFormat{
		version: httpFormatVersion2,
	}
}

the other option is to 'With*' style option to configure

func NewHTTPFormat(o Options...) {
}

Options approach may be too much as I don't expect more options in the future.

rghetia avatar Apr 08 '20 23:04 rghetia

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?

punya avatar Apr 14 '21 11:04 punya