spandex-project / spandex

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

Feature: GenServer for sending traces asynchronously. #16

Closed driv3r closed 7 years ago

driv3r commented 7 years ago

We can compare now sync vs async, although it's probably better to just get rid of old API adapter.

Also one more thing, it's probably better to improve a structure a bit, and move everything under lib/spandex/ as it's encouraged when you create new mix app.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+2.7%) to 64.948% when pulling aac4e17992f5adb07475b0fa2b155c797fc591c7 on driv3r:feature/11-genserver-for-sending-traces into 6a8fa98ef528b3910f82e67c233926c7ada3642c on zachdaniel:master.

zachdaniel commented 7 years ago

This is going to take me some time to review. I've only ever had bad experiences using tools like VCR (in rails) and I would have to think very hard before leaning into one for this, especially when testing against a closed source API. With that said, I understand the benefits and relative ease of use of that kind of tool. I'll have to think about it.

driv3r commented 7 years ago

Couple of questions:

  1. Do we need to keep the original ApiAdapter or should we just default to new one?
  2. I'm open for other ways of testing ApiServer, so far I know just vcr for this kind of stuff in elixir (in ruby land I don't use it either ;)
  3. Regarding the runtime configuration, do you actually know the cases of such runtime changes? Tracing is supposed to be super lightweight in order to not add too much of an overhead, that's why I think we should come up with a better solution than blindly fetching env vars with each span which can definitely add up. If it's necessary to have this ability of live updates via env vars, then I would probably go with config cache via gen server worker, where we could store compiled configuration, and every x seconds/minutes check for updates, wdys?
  4. If we want to make this lib vendor agnostic then we should base the API on OpenTracking API which is probably what DD is based on anyway.
driv3r commented 7 years ago

btw. Tomorrow I will do some testing and check how it works compared to synchronous version.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+2.7%) to 64.948% when pulling 4ccf930df13d5673f215b0301fe3cce521a4e116 on driv3r:feature/11-genserver-for-sending-traces into 6a8fa98ef528b3910f82e67c233926c7ada3642c on zachdaniel:master.

zachdaniel commented 7 years ago

So, for configuration, what we should do is store the configuration in a gen_server or an agent, but use Confex to get it originally. Then, when someone starts a trace, we ask the gen_server for the starting config, and store that in the process dictionary with the other trace data. @driv3r what are your thoughts on that?

driv3r commented 7 years ago

@zachdaniel I think it makes sense then each trace have a copy of config for its lifespan, so even if ENV var will change during runtime, all spans that started before will get finished.

One more questions needs to be answered before I could finish this up, do we leave or remove ApiAdapter?

driv3r commented 7 years ago

maybe instead of swapping the ApiAdapter we could swap just the HTTPoison?

zachdaniel commented 7 years ago

I like the ApiAdapter. It exists to fill the same niche that VCR fills, but in a simpler way (IMO). I'm actually curious, with the TestApiAdapter, what requests were being made that had to be mocked with VCR?

driv3r commented 7 years ago

vcr was used in order to test ApiServer, and full path from creation of span to http put. We can ditch tests for ApiServer and use TestApiAdapter in full path instead although then it doesn't make sense for full path :)

coveralls commented 7 years ago

Coverage Status

Coverage increased (+7.7%) to 70.0% when pulling 97e3c4ee79c448ab6055db790a14f0dbbe6e7625 on driv3r:feature/11-genserver-for-sending-traces into 6a8fa98ef528b3910f82e67c233926c7ada3642c on zachdaniel:master.

zachdaniel commented 7 years ago

@driv3r This looks good. I should have added a test for it, but I was in a bit of a hurry. Thanks, also for moving away from VCR.