move default attributes to separate macro

[tsloughter]

I don't really like this, but it is the only thing I've been able to come up with so far.

Because traces should have a near zero cost I am hesitant to have anything included by default that is not absolutely necessary to fulfill the spec.

In a perfect world there would be a way for at run time the user to define what attributes from these defaults are to be included in each span, but I don't know of one that doesn't involve looking up application env data or some other ets table (maybe persistent_term would work in the future).

Open to alternatives.

[codecov-io]

Codecov Report

Merging #5 into master will increase coverage by 100%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #5     +/-   ##
=======================================
+ Coverage       0%   100%   +100%     
=======================================
  Files           1      1             
  Lines           5      5             
=======================================
+ Hits            0      5      +5     
+ Misses          5      0      -5

Impacted Files	Coverage Δ
lib/opencensus/trace.ex	100% <ø> (+100%)	⬆️

---

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 2658253...03c00ca. Read the comment docs.

[hauleth]

I am not sure about distinction. I would rather allow setting attributes to %{file: nil} to prevent it from sending to the reporter. Alternatively we could make default attributes default and with_child_span_naked/2-3 macro for handling it without defaults.

[deadtrickster]

I'm all for explicitness in cases like this. I feel like with more macrology we achieve the same without having two macros. Having two macros is a compile-time dispatch anyway. So maybe we could have an optional parameter, like include_default_attributes: nil. Or allowing attributes argument to be also a tuple/list like {(:line, :module}, } or [:line, :module] or [:line, :module, map].

with_child_span("my-important-span", [:line, :module]) do
end

## or

with_child_span("my-important-span", [:line, :module, %map]) do
end

[tsloughter]

Hm, I think I like that idea of a list of attributes you can tell it to include, and maybe in addition to individual ones like :line there could be an :all.

[deadtrickster]

yep, like I have :default as a shortcut for all built in prometheus collectors

[tsloughter]

@deadtrickster I'm not sure how to do it besides a naive way. Got any pointers on what this should look like to be a macro that the code it generates only builds a map of the requested values, instead of building the whole map and filtering or always iterating over all possibilities and matching on the ones requested?

Figure as a macro it should be able to just output code that only does anything with the requested attributes.

[deadtrickster]

If I understand you correctly, I would do it like this:

if list, iterate and find if a map is here, if it is - use, otherwise 
map = %{}.
quote do
  map = unquote(map)
  unquote_splicing(
     collect-iterate over attributes again (skipping the map)  do
	    quote do 
          map = Map.put(map, unquote(known_key), unquote(gen_attr_value(known_key))
        end
	 end
  )
end

Highlights - iterate at compile time, Map.put only requested stuff. should be a var not map itself. We initialize it with provided attributes map, if any.
Probably can get rid of Map.put too, just quoting map literal builder.

[tsloughter]

I gave up on getting the list idea to work. So I've tried something different that is simply an additional macro (default_attributes) you can pass an optional map to and pass to with_child_span.

If no one is happy with this one either then I'll revert to the old way of defaulting to having all the attributes and publish as is.

This version makes me think it should be offered to the Erlang version as well. have:

-define(ATTRS(Attributes), Attributes#{module => ?MODULE, function => ...}).

[hauleth]

@tsloughter I was thinking more about macro in form of:

-define(WITH_CHILD_SPAN(Name, Body), ocp:with_child_span(Name, #{
    module => ?MODULE,
    line => ?LINE,
    file => ?FILE,
    function => lists:concat([?FUNCTION_NAME, "/", integer_to_list(?FUNCTION_ARITY)]},
    fun() -> Body end)).

[hauleth]

You can use Elixir's Application module to be more idiomatic.

[tsloughter]

@deadtrickster @hauleth thoughts on this latest PR?

@deadtrickster were you able to try your idea?

[deadtrickster]

No, mad week, tomorrow.

[hauleth]

I am not fan of this approach, I would much better like the "Elixir logger" approach, where we can pass nil for values we want to be removed from the metadata instead of forcing user to pass "default parameters" in form of additional call, as this makes them not default.