standardize return types from strategies (and by proxy from Spandex) #62

zachdaniel · 2018-08-23T19:09:40Z

@GregMefford mentioned that a lot of the return types in the inner parts (and some of the public interface) were not conventional. This updates the ones from the strategy behaviour to follow that convention, and adapts much of the spandex code to handle that.

zachdaniel · 2018-08-26T17:41:38Z

My auto formatter made a few changes, but they are trivial. This PR also changes opts[:tracer] to opts[:trace_key] which makes it a bit more informative (not much) as to what that key is for. Its really just a way to have multiple traces running at the same time. So if you wanted only one trace, and you didn't want to use a tracer, you could just do Spandex.start_trace("hello") and everything would work fine (the trace_key would just be nil). You only have to care about trace_key if you want more than one active trace at a time, at which point you'd say Spandex.start_trace("hello", trace_key: "foobar")

zachdaniel · 2018-08-26T17:41:54Z

@GregMefford if you could take a look, that would be great!

GregMefford

This looks good other than the proposal/question about splitting out another @callback in the Strategy behaviour. 👍

I'm surprised that your formatter did something different than mine, unless you're not using plain old mix format. Maybe I forgot to run mine after the last round of tweaks.

GregMefford · 2018-08-26T18:29:29Z

lib/strategy.ex

@@ -6,7 +6,7 @@ defmodule Spandex.Strategy do

  alias Spandex.Trace

-  @callback get_trace(tracer) :: Trace.t() | nil
+  @callback get_trace(tracer) :: {:ok, Trace.t() | nil} | {:error, term}


What do you think about adding another @callback trace_active?(tracer) function that would return a true/false, since that's mostly what Spandex ends up doing with the return value of this function. It might be possible to more-efficiently implement that function and avoid a little work in many cases where the result ends up not getting used.

If we were to do that, I'd propose that we return {:error, :no_trace_context} here instead of {:ok, nil} when there's no active trace, given that you'd only call this function when you actually expect to use the result.

Lets do it 👍

Great suggestion :)

zachdaniel · 2018-08-26T18:47:58Z

I'm pretty sure it should just be the plain mix format + the .formatter.exs of the project. If not, it would have shortened the lines. Would be nice to get travis figured out and just check formatting in PR. Still haven't figured out how on earth to actually get the github apps/marketplace version of travis CI running against this org.

GregMefford · 2018-08-26T19:22:52Z

lib/spandex.ex


-      trace ->
+      {:error, _} = error ->
+        error


Since this kind of error would be up to the Strategy, maybe we should have something like:

{:error, reason} -> Logger.error("Failed to start span: #{reason}") {:error, reason}

GregMefford · 2018-08-26T19:23:26Z

lib/spandex.ex

        do_update_span(trace, opts, top?)
+
+      {:error, _} = error ->
+        error


Same thing about logging an error here.

GregMefford · 2018-08-26T19:23:53Z

lib/spandex.ex

+        strategy.put_trace(opts[:trace_key], %{trace | stack: new_stack, spans: new_spans})
+
+      {:error, _} = error ->
+        error


Same thing about logging an error here.

GregMefford · 2018-08-26T19:24:03Z

lib/spandex.ex

+        strategy.delete_trace(opts[:trace_key])
+
+      {:error, _} = error ->
+        error


Same thing about logging an error here.

GregMefford · 2018-08-26T19:24:19Z

lib/spandex.ex

        {:ok, finished_span}
+
+      {:error, _} = error ->
+        error


Same thing about logging an error here.

GregMefford · 2018-08-26T19:27:17Z

lib/spandex.ex

+            trace
+            | stack: tail,
+              spans: [finished_span | spans]
+          })


I don't see the purpose of having _ = ... at the beginning of the line here.
Also, this formatting seems kind of weird to me, but it might only be doing that because of the _ = making it want to use multiple lines.

I have a really strict dialyzer configuration in my work projects, which includes "no unmatched returns" so thats just a holdover from the way I'm used to working. I didn't mean to add it back in after you removed them, it just happened from resolving merge conflicts I think.

GregMefford · 2018-08-26T19:31:30Z

lib/spandex.ex

@@ -289,7 +320,8 @@ defmodule Spandex do
    adapter = opts[:adapter]

    with {:ok, span} <- Span.child_of(current_span, name, adapter.span_id(), adapter.now(), opts),
-         {:ok, _trace} <- strategy.put_trace(opts[:tracer], %{trace | stack: [span | trace.stack]}) do
+         {:ok, _trace} <-
+           strategy.put_trace(opts[:trace_key], %{trace | stack: [span | trace.stack]}) do


When I pull this branch and run mix format, it puts this line back to the way it was before this PR. Maybe your formatter isn't picking up the config file that allows 120 characters?

Yeah, I think something is up with my editor.

sourcelevel-bot · 2018-08-26T20:00:29Z

lib/spandex.ex

+        span
+
+      {:error, _} ->
+        # TODO: Alter the return type of this interface to allow for returning


sourcelevel-bot · 2018-08-26T20:00:30Z

lib/spandex.ex

        id
+
+      {:error, _} ->
+        # TODO: Alter the return type of this interface to allow for returning


sourcelevel-bot · 2018-08-26T20:00:30Z

lib/spandex.ex

+        span
+
+      {:error, _} ->
+        # TODO: Alter the return type of this interface to allow for returning


Found a TODO tag in a comment: # TODO: Alter the return type of this interface to allow for returning

sourcelevel-bot · 2018-08-26T20:00:30Z

lib/spandex.ex

        id
+
+      {:error, _} ->
+        # TODO: Alter the return type of this interface to allow for returning


Found a TODO tag in a comment: # TODO: Alter the return type of this interface to allow for returning

zachdaniel · 2018-08-26T20:36:44Z

@GregMefford WRT to the logging, I’m not sure we should favor having logging in place at the moment. At least not without a toggle for the level and/or a toggle to turn it on/off. At scale, logging can be a significant bottleneck for many applications, and if there is a problem with their spandex configuration, we don’t want to have a bunch of log statements. Basically we just need some way to ensure that spandex isn’t going to log 10 additional times per request (which is a pretty conservative number of spans that a lot of people might have in a given request).

sourcelevel-bot · 2018-08-27T15:41:29Z

Ebert has finished reviewing this Pull Request and has found:

4 possible new issues (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/spandex-project/spandex/pulls/62.

coveralls · 2018-08-27T15:44:20Z

Pull Request Test Coverage Report for Build 317

29 of 35 (82.86%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.3%) to 82.278%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/spandex.ex	25	31	80.65%

Totals
Change from base Build 313:	-1.3%
Covered Lines:	195
Relevant Lines:	237

💛 - Coveralls

GregMefford mentioned this pull request Aug 24, 2018

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

Merged

zachdaniel force-pushed the standardize-strategy-return-types branch from c05087b to 7397212 Compare August 26, 2018 17:38

GregMefford reviewed Aug 26, 2018

View reviewed changes

sourcelevel-bot bot reviewed Aug 26, 2018

View reviewed changes

standardize return types from strategies (and by proxy from Spandex)

29dd221

zachdaniel force-pushed the standardize-strategy-return-types branch from 45f3f38 to 29dd221 Compare August 27, 2018 15:40

fix opt name

4db40e2

zachdaniel merged commit d9e2040 into master Aug 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

standardize return types from strategies (and by proxy from Spandex) #62

standardize return types from strategies (and by proxy from Spandex) #62

zachdaniel commented Aug 23, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

GregMefford left a comment

GregMefford Aug 26, 2018

zachdaniel Aug 26, 2018

zachdaniel Aug 26, 2018

zachdaniel commented Aug 26, 2018

GregMefford Aug 26, 2018

GregMefford Aug 26, 2018

GregMefford Aug 26, 2018

GregMefford Aug 26, 2018

GregMefford Aug 26, 2018

GregMefford Aug 26, 2018

zachdaniel Aug 26, 2018

GregMefford Aug 26, 2018

zachdaniel Aug 26, 2018

sourcelevel-bot bot Aug 26, 2018

sourcelevel-bot bot Aug 26, 2018

sourcelevel-bot bot Aug 26, 2018

sourcelevel-bot bot Aug 26, 2018

zachdaniel commented Aug 26, 2018

sourcelevel-bot bot commented Aug 27, 2018

coveralls commented Aug 27, 2018 •

edited

Loading

standardize return types from strategies (and by proxy from Spandex) #62

standardize return types from strategies (and by proxy from Spandex) #62

Conversation

zachdaniel commented Aug 23, 2018

zachdaniel commented Aug 26, 2018

zachdaniel commented Aug 26, 2018

GregMefford left a 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 26, 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

sourcelevel-bot bot Aug 26, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot Aug 26, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot Aug 26, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot Aug 26, 2018

Choose a reason for hiding this comment

zachdaniel commented Aug 26, 2018

sourcelevel-bot bot commented Aug 27, 2018

coveralls commented Aug 27, 2018 • edited Loading

Pull Request Test Coverage Report for Build 317

💛 - Coveralls

coveralls commented Aug 27, 2018 •

edited

Loading