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

adds set_kind func for oc_trace #167

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

Conversation

castengo
Copy link

No description provided.

@@ -53,10 +53,13 @@ modifications(_Config) ->
Span6 = oc_span:add_link(Link, Span5),
?assertEqual(undefined, oc_span:add_link(Link, undefined)),

?assertEqual({error, no_report_buffer}, oc_span:finish_span(#span_ctx{}, Span6)),
Span7 = oc_span:set_kind(<<"SERVER">>, Span6),
Copy link
Member

Choose a reason for hiding this comment

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

span_kind() is an atom not a binary.

Copy link
Author

Choose a reason for hiding this comment

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

🤦🏼‍♀️

@tsloughter
Copy link
Member

My first thought was that maybe the spec said kind could only be set when a span is started. I can't find that in the spec so I guess this is fine, only issue is the test using a binary instead of an atom.

@castengo
Copy link
Author

yeah we are starting the trace with_child_span since its a remote trace, so I could also add it as an option to that func if you think its better.

I'll change that to an atom, thanks for reviewing!

@castengo castengo force-pushed the set-kind branch 2 times, most recently from 4639e04 to b522ef0 Compare August 14, 2020 00:46
%%--------------------------------------------------------------------
-spec span_kind_server() -> span_kind().
span_kind_server() -> ?SPAN_KIND_SERVER.

Copy link
Author

Choose a reason for hiding this comment

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

need this for elixir lib

Copy link
Member

Choose a reason for hiding this comment

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

Cool, can you add it for all the kinds and then i'll merge.

Copy link
Author

Choose a reason for hiding this comment

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

👍

Copy link
Author

Choose a reason for hiding this comment

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

updated!

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.

2 participants