spandex-project / spandex

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

`distributed_context` takes a `Plug.Conn` rather than `headers()` #112

Closed jfmyers9 closed 4 years ago

jfmyers9 commented 4 years ago

Currently there are two methods that Spandex provides to do distributed tracing.

inject_context:

  @spec inject_context(headers(), SpanContext.t(), Tracer.opts()) :: headers()

distributed_context:

  @spec distributed_context(Plug.Conn.t(), Tracer.opts()) ::
          {:ok, SpanContext.t()}
          | {:error, :disabled}

Given that these two methods are intended to be counterparts of each other, would it make more sense if the distributed_context interface was actually:

  @spec distributed_context(headers(), Tracer.opts()) ::
          {:ok, SpanContext.t()}
          | {:error, :disabled}

An example use case would be injecting and extracting tracing contexts from a GRPC request which does not rely on Plug.Conn.

Thoughts?

zachdaniel commented 4 years ago

Probably, yeah. In the interest of backwards compatibility, though, we could just do:

def distributed_context(headers, opts) when is_list(headers)

And just make it accept a conn or headers

jfmyers9 commented 4 years ago

Makes sense. This would require Adapters to update their interfaces as well. What would be the expected behavior if a developer used a new version of Spandex with an older version of an Adapter? Would a failure in this situation be ok?

zachdaniel commented 4 years ago

They wouldn't have to update if we accept a list of headers or a conn, and return the same result, right? Everything should be hunky-dory.

jfmyers9 commented 4 years ago

Unfortunately at the moment we just forward the Plug.Conn to the adapter. I'm not sure how we would be able to do this without updating the adapters without doing something odd like creating an empty Plug.Conn that contains the headers.

zachdaniel commented 4 years ago

Yeah, you're right. Right now, there is only one supported adapter (unless someone made a custom one). So: 1.) We release the change we discussed here to mainline spandex as a major release 2.) We release the change to spandex_datadog as a major release, and bump the dependency in mix.exs to the new major release in spandex

This should solve for the case that someone updates spandex or spandex_datadog w/o updating the other one. I don't mind doing the releases, if you're interested in PR-ing the changes to the two projects 👍

jfmyers9 commented 4 years ago

Sounds like a good plan. I should be able to whip up a PR soon. Thanks for the input.