spandex-project / spandex

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

Rework abstraction to prepare for more adapters #18

Closed zachdaniel closed 6 years ago

zachdaniel commented 7 years ago

Currently, we break from the abstract at a high level, basically just defining the interface of a tracer. I'm getting started adding a google stackdriver tracing adapter, and what I'm realizing is that pretty much all of the logic stays the same. The only thing that is different is ApiServer, and a few small things (like trace_id/span_ids). So here is the proposition: We make most of the Datadog adapter code only adapter code, and make Spandex.Datadog.Span just Spandex.Span. Then, we define an adapter in terms of the small things it needs to change about that process.

defmodule Spandex.Adapters.Adapter do
  @moduledoc """
  This isn't all the callbacks, just a starting point.
  """
  @callback new_id() :: term
  @callback now() :: term
  @callback send_trace() :: term
end

@driv3r @asummers What do you guys think?

asummers commented 7 years ago

Tentatively into it. Though, and this sort of aligns with what you're suggesting, it might be better to have a Spandex representation of span and trace, and have the vendor specific adapters apply post processing rules to convert the payloads into something that vendor can use. For example we could store timestamps internally however we want, and have the DataDog adapter do the conversion and truncation of precision. That would lead to a cleaner implementation of the core logic, I think.

driv3r commented 7 years ago

same here, and also same regarding the logic inside vendors Span I would say it should mostly have only to_map/encode methods, ew. depending on what's the format of IDs for specific vendors it could either delegate it to spandex or implement custom one.

Besides that I think that the ApiServer is good to keep separate, ew. we could abstract the Server part, and have implementation of encoding & sending at vendor level, this way we always initialize same gen server, where we give vendor module as config option. this would probably simplify logging, part with broadcast, etc. as at the moment the only part in ApiServer regarding DD is [spans] |> encode |> push(state) (btw. this would also allow us to have a pool of servers, or even something like gen stage without worrying about implementations too much)

driv3r commented 7 years ago

Also I would suggest taking a look at OpenTracing which is already defined standard used by several vendors.

zachdaniel commented 7 years ago

@asummers I think that timestamps are an example of why we might not want to store timestamps in our own fashion. Imagine someone wants to add an adapter that has more precise timestamps than our internal representation? I think most things should be represented as post processing, but timestamps and id generation would need to be embedded. Imagine the process of replacing all of our internal ids (and then having to find any child span with the id you just replaced, and update it). In general I agree, though.

zachdaniel commented 7 years ago

@driv3r I think we're on the same page :) I'll see if I can write up a bunch of tickets to break this up into smaller pieces of work.

zachdaniel commented 6 years ago

This is superceded by new incoming work.