Fix dialyzer issues and use :ok tuples when :error tuples are possible #63

GregMefford · 2018-08-24T02:32:41Z

When running mix dialyzer, I was getting the following errors:

lib/span.ex:87: Function new/1 has no local return
lib/span.ex:88: The call 'Elixir.Spandex.Span':update(#{'__struct__'=>'Elixir.Spandex.Span', 'completion_time'=>'nil', 'env'=>'nil', 'error'=>'nil', 'http'=>'nil', 'id'=>'nil', 'name'=>'nil', 'parent_id'=>'nil', 'private'=>'nil', 'resource'=>'nil', 'service'=>'nil', 'sql_query'=>'nil', 'start'=>'nil', 'tags'=>'nil', 'trace_id'=>'nil', 'type'=>'nil'},_opts@1::any(),#{'__struct__':='Elixir.Optimal
.Schema', 'annotations':=[], 'custom':=[], 'defaults':=[{'private',[]} | {'services',[]} | {'tags',[]},...], 'describe':=[], 'extra_keys?':='true', 'opts':=[atom(),...], 'required':=['env' | 'id' | 'name' | 'service' | 'start' | 'trace_id',...], 'types':=[{atom(),'any' | 'atom' | 'keyword' | 'string' | ['atom' | 'string',...]},...]}) will never return since it differs in the 1st argument from th
e success typing arguments: (#{'__struct__':='Elixir.Spandex.Span', 'completion_time':=_, 'env':=binary(), 'error':='nil' | [{_,_}], 'http':='nil' | [{_,_}], 'id':=_, 'name':=binary(), 'parent_id':=_, 'private':=[{_,_}], 'resource':=binary(), 'service':=atom(), 'sql_query':='nil' | [{_,_}], 'start':=_, 'tags':='nil' | [{_,_}], 'trace_id':=_, 'type':=atom()},any(),#{'__struct__':='Elixir.Optimal.
Schema', 'annotations':=[{_,_}], 'custom':=[{_,_}], 'defaults':=[{_,_}], 'describe':=[{_,_}], 'extra_keys?':=boolean(), 'opts':=[atom()], 'required':=[atom()], 'types':=[{_,_}]})
lib/spandex.ex:264: Function span/4 has no local return

Those are now resolved, but when I started to tug on that thread, the changes ended up leading me all over the place and I'm sorry for the huge PR.

As mentioned in #62, this also standardizes on {:ok, result} tuples wherever an {:error, reason} tuple is possible. For that reason, I'm proposing a version bump from 2.0.0 to 2.1.0 since it's a small breaking change to the API, but my guess is that people probably aren't often using the return values from these functions in practice, as long as the right things are getting passed around inside Spandex.

…sible

GregMefford · 2018-08-24T02:37:32Z

I also put in a commit to set the formatter line_length to 120 because the default of 80 isn't very wide and makes some lines split in annoying ways. I'm fine if you don't want to change that, though.

asummers · 2018-08-24T02:51:39Z

Should pull in the newest Dialyxir RC while you're here to get the new Elixir formatting =)

zachdaniel · 2018-08-24T03:21:17Z

lib/adapter.ex

-  @callback now() :: term
-  @callback default_sender() :: module
+  @callback trace_id() :: Spandex.id()
+  @callback span_id() :: Spandex.id()


So the adapter is in charge of the type, meaning we shouldn't set it to be non_neg_integer

I actually wanted to propose that we change it to String.t() so that the adapter can choose, but it was currently an integer so I wanted to do that as a separate PR.

But it's explicitly not a string for datadog and others. We really can't set a type in this library otherwise the adapters will fail dialyzer and/or they will have to be bound by whatever type we set.

I was thinking that Spandex says it's a String.t() and the adapters can serialize/deserialize however they want to. I imagine most back-ends will use a UUID - I was surprised that DataDog just uses a random integer.

I don't really see a benefit to forcing serialization/deserialization on the adapters end. Nothing in core cares if they are a string, so why bother forcing it? We should just let it be whatever type their adapter uses.

Yeah, I guess that's fair. 👍

Ok, I defined Spandex.id() to be term() so that we can still document the fact that a function expects a Spandex.id(), and still leave it up to the Adapters, without having to sprinkle term() all over the place.

Yeah, that's a good plan :)

zachdaniel · 2018-08-24T03:24:48Z

lib/span.ex

-    update(%__MODULE__{}, opts, @span_opts)
+    case Optimal.validate(opts, @span_opts) do
+      {:ok, opts} ->
+        span = Map.merge(%Span{}, Enum.into(opts, %{}))


This is a pretty radical change from calling update I'm pretty positive that update contains transformations that we shouldn't drop here. Additionally, we'd want to just do struct(Span, opts)

I think this ought to be left calling into update.

The reason I changed it is that dialyzer was unhappy about passing the struct into the update function with all nil values (because the type def says that’s not allowed). I’ll think about whether there’s a different way to solve that without just allowing all the fields to be nil (which seems like a cop-out to me).

I agree that it is a copout, but it's definitely losing functionality by not calling into update, so we'll have to figure something out. Perhaps just extracting the main parts ofthe update function into their own functions and calling those from create?

zachdaniel · 2018-08-24T03:26:20Z

mix.exs

@@ -4,7 +4,7 @@ defmodule Spandex.Mixfile do
  def project do
    [
      app: :spandex,
-      version: "2.0.0",
+      version: "2.1.0-dev",


We'll bump the versions separately afterwards, as opposed to in this PR.

zachdaniel · 2018-08-24T03:34:13Z

lib/spandex.ex

  def distributed_context(_, :disabled), do: {:error, :disabled}

  def distributed_context(conn, opts) do
    adapter = opts[:adapter]
    adapter.distributed_context(conn, opts)
  end

+  # Private Helpers
+
+  defp do_continue_trace(name, trace_id, opts) do


Hard to cross reference visually. None of the bodies in do_* are altered from their originals, right? Same as they were when they were inline above?

I’ll double check and make sure we have decent test coverage on them before we merge. One reason to do this (besides making the functions simpler to understand) was that dialyzer was having trouble following along when there is a with inside an if/else.

Yeah, definitely a positive change, just wanted to make sure I didn't miss any changes.

Added a bunch of tests, so I think this refactor should be relatively safe.
Note that there are some tests with @tag :skip because they don't currently validate additional opts, but I wasn't sure whether that was intentional.

zachdaniel · 2018-08-24T03:35:22Z

@asummers lets do a separate PR with that. Feel free to, or to lodge an issue for me to do it :)

zachdaniel · 2018-08-24T03:37:23Z

This looks great! Left some feedback/discussion.

GregMefford · 2018-08-24T14:58:11Z

Hadn't heard about that @asummers, but now I want to look into that! 😁

zachdaniel · 2018-08-26T01:54:14Z

The testing looks awesome. I'm very surprised to hear the opts with invalid types are ignored. If that's true, it's 100% a bug that we should fix. I still have a slight issue with Span.new I think. When I'm at my laptop I'll post an alternative. Aside from that, and the issue w/ validation, I think this should be good to go.

zachdaniel · 2018-08-26T01:57:38Z

We should diagnose the validation issue at least, and then maybe fix it in a separate PR.

zachdaniel · 2018-08-26T02:20:47Z

Ah, I see the issue. The validator is doing the right thing, but we always return the span, even if we didn't update it. I think that, as part of this return type refactor, we would expect to return the errors.

zachdaniel · 2018-08-26T02:23:20Z

Thinking about it now, there are certain functions where that would be hard (like update_all_spans). I think that, for now, we should leave the tests you've added in place, but have them skipped. I will add a ticket now to investigate replacing the usage of update_or_keep with something that would provide more meaningful returns. We will probably just have to have update_all_spans always return a success, or not update anything in the case that any of them are an error. Either way, probably should not address here.

zachdaniel · 2018-08-26T02:44:17Z

Alright, @GregMefford since we know whats up with the validation issue the only thing left is Span.new/2. This PR #66 proposes changes to this branch to address my concerns. Once we're done with that, we should be good to go.

GregMefford · 2018-08-26T16:14:58Z

Ok, I think this is all set. ✅

Fix dialyzer issues and use :ok tuples whenever :error tuples are pos…

20f7e28

…sible

GregMefford requested a review from zachdaniel August 24, 2018 02:32

Allow the formatter to use up to 120 columns

7b04050

zachdaniel reviewed Aug 24, 2018

View reviewed changes

Integrate PR feedback

361d41d

zachdaniel mentioned this pull request Aug 26, 2018

Explore removing update_or_keep/2 from Spandex.Span #65

Open

GregMefford and others added 2 commits August 26, 2018 12:13

Add tests for Spandex module and fix inconsistencies

33b3507

Tweaking the dialyzer fixes for Span.new()

e7e6713

GregMefford force-pushed the fix_dialyzer_errors branch from 5c52377 to e7e6713 Compare August 26, 2018 16:14

zachdaniel approved these changes Aug 26, 2018

View reviewed changes

zachdaniel merged commit a31b838 into master Aug 26, 2018

GregMefford deleted the fix_dialyzer_errors branch August 26, 2018 17:17

GregMefford restored the fix_dialyzer_errors branch August 26, 2018 19:06

GregMefford deleted the fix_dialyzer_errors branch August 26, 2018 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dialyzer issues and use :ok tuples when :error tuples are possible #63

Fix dialyzer issues and use :ok tuples when :error tuples are possible #63

GregMefford commented Aug 24, 2018

GregMefford commented Aug 24, 2018

asummers commented Aug 24, 2018

zachdaniel Aug 24, 2018

GregMefford Aug 24, 2018

zachdaniel Aug 24, 2018

GregMefford Aug 25, 2018

zachdaniel Aug 25, 2018

GregMefford Aug 25, 2018

GregMefford Aug 26, 2018

zachdaniel Aug 26, 2018

zachdaniel Aug 24, 2018

zachdaniel Aug 24, 2018

GregMefford Aug 24, 2018

zachdaniel Aug 24, 2018

zachdaniel Aug 24, 2018

zachdaniel Aug 24, 2018

GregMefford Aug 24, 2018

zachdaniel Aug 24, 2018

GregMefford Aug 26, 2018

zachdaniel commented Aug 24, 2018

zachdaniel commented Aug 24, 2018

GregMefford commented Aug 24, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

GregMefford commented Aug 26, 2018

Fix dialyzer issues and use :ok tuples when :error tuples are possible #63

Fix dialyzer issues and use :ok tuples when :error tuples are possible #63

Conversation

GregMefford commented Aug 24, 2018

GregMefford commented Aug 24, 2018

asummers commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachdaniel commented Aug 24, 2018

zachdaniel commented Aug 24, 2018

GregMefford commented Aug 24, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

GregMefford commented Aug 26, 2018