spandex-project / spandex

A platform agnostic tracing library
MIT License
335 stars 53 forks source link

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

Closed zachdaniel closed 6 years ago

zachdaniel commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

zachdaniel commented 6 years ago

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.

zachdaniel commented 6 years ago

@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[bot] commented 6 years ago

Ebert has finished reviewing this Pull Request and has found:

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

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 317


Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/spandex.ex 25 31 80.65%
<!-- Total: 29 35 82.86% -->
Totals Coverage Status
Change from base Build 313: -1.3%
Covered Lines: 195
Relevant Lines: 237

💛 - Coveralls