Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add X-MoneySampled header to MoneyTraceFormatter to convey sampled state #158

Closed
wants to merge 2 commits into from

Conversation

HaloFour
Copy link
Collaborator

@HaloFour HaloFour commented Oct 26, 2020

Adds an optional X-MoneySampled headers with a value of 0 or 1 to convey whether the remote span is sampled.

Another option would be to add another key-value pair to the end of the existing header but that will break the Go Money implementation which assumes that there can only be three key-value pairs.

@codecov-io
Copy link

Codecov Report

Merging #158 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage   93.61%   93.64%   +0.03%     
==========================================
  Files          77       77              
  Lines        1096     1102       +6     
  Branches       80       73       -7     
==========================================
+ Hits         1026     1032       +6     
  Misses         70       70              
Impacted Files Coverage Δ
...st/money/core/formatters/MoneyTraceFormatter.scala 100.00% <100.00%> (ø)

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 0a310b2...9c87599. Read the comment docs.

@HaloFour
Copy link
Collaborator Author

I've opened this issue with golang-money to inquire about modifying the existing X-MoneyTrace header instead of adding a second header:

xmidt-org/golang-money#43

@HaloFour
Copy link
Collaborator Author

TL;DR:

Both the Scala and Go versions of Money expect a header with the name X-MoneyTrace. The header contains a series of name/value pairs delimited by ; where the key and value is split by a single =.

The Scala version requires a minimum of three key/value pairs, but only inspects the first three pairs. It does not inspect the names of the keys at all, rather it assumes that the pairs will always be in the order of trace-id, parent-id and span-id.

Conversely, the Go version requires exactly three key/value pairs. However, does inspect the names of the keys to determine which is the trace-id, parent-id and span-id. It must see each of the keys exactly once. Because of these rules it cannot accept additional key/value pairs.

The only way to keep the Scala and Go versions of Money compatible with each other and previous versions would be to leave the X-MoneyTrace header alone. It will always emit three key/value pairs and they will always be in the order trace-id, parent-id and span-id.

@HaloFour
Copy link
Collaborator Author

I'm going to close this PR in favor of #159

@HaloFour HaloFour closed this Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants