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

convert annotations to string values in zipkin reporter #104

Merged

Conversation

tsloughter
Copy link
Member

No description provided.

@codecov-io
Copy link

codecov-io commented Oct 6, 2018

Codecov Report

Merging #104 into master will increase coverage by 0.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
+ Coverage    80.6%   80.62%   +0.01%     
==========================================
  Files          34       34              
  Lines         696      707      +11     
==========================================
+ Hits          561      570       +9     
- Misses        135      137       +2
Impacted Files Coverage Δ
src/oc_reporter_zipkin.erl 78.94% <81.81%> (+1.16%) ⬆️

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 acce869...46f1574. Read the comment docs.

uncompressed_size=UncompressedSize,
compressed_size=CompressedSize}) ->
iolist_to_binary(["MessageEvent:{type=", atom_to_binary(Type, utf8),
", id=", integer_to_binary(Id),
Copy link
Contributor

Choose a reason for hiding this comment

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

integer_to_binary - converts to decimal value or binary? real binary may be hard to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is more like integer to string. Erlang has a specific binary type. It is used a lot for strings, esp when going to json since regular Erlang strings are just lists of integers and the way most json encoders work in Erlang is to require strings as binaries so it can easily know if you meant a string or an array of integers.

Binaries are also represented with the << and >> syntax you probably see all over the code.

@deadtrickster
Copy link
Member

do you have tests too?

@tsloughter
Copy link
Member Author

Adding a test now.

@tsloughter
Copy link
Member Author

Updated with test.

@tsloughter tsloughter merged commit 31027f4 into census-instrumentation:master Oct 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants