Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anovitskiy
Copy link

@anovitskiy anovitskiy commented Mar 25, 2020

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

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%) ⬇️

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

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

@rghetia
Copy link
Contributor

rghetia commented Apr 2, 2020

@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
Copy link
Author

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

rghetia commented Apr 8, 2020

@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
Copy link
Author

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

rghetia commented Apr 8, 2020

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.

@punya
Copy link
Contributor

punya commented Apr 14, 2021

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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants