Implemented one endpoint for span creation - Span.new/1

spandex-project / spandex

A platform agnostic tracing library

MIT License

335 stars 53 forks source link

Implemented one endpoint for span creation - Span.new/1 #9

Closed driv3r closed 7 years ago

driv3r commented 7 years ago

This way we can ensure that we always set sane defaults based on configuration settings.

Wondering if we should also apply Span.new/1 in Span.child_of/1?
We should probably use %Span{} struct everywhere instead of plain map, wdys?
Any other places to look for?

Fixes #7

[x] Span.new/1 constructor with defaults
[x] clean up adapter from Span.begin(new())
[x] clean up Plug.* modules
[x] check if we actually trace request via Plug.Conn.assigns
[x] improve test coverage for Span
[x] improve test coverage for Plug.* modules

coveralls commented 7 years ago

Coverage increased (+2.2%) to 44.681% when pulling 11decb014c6b6c6f009733dab93407f3536b0f58 on driv3r:bugfix/7-use-config-for-defaults into 6fc831b7a90bcfcbbac0dddf136c9bb8f1d3146f on zachdaniel:master.

coveralls commented 7 years ago

Coverage increased (+1.7%) to 44.17% when pulling a670c609e12847d986421a9e4334c18c7a6a8229 on driv3r:bugfix/7-use-config-for-defaults into 6fc831b7a90bcfcbbac0dddf136c9bb8f1d3146f on zachdaniel:master.

zachdaniel commented 7 years ago

I would also check the add context plug to make sure that it behaves well w/ the defaults

driv3r commented 7 years ago

I wonder if the test failure we're seeing here implies that we're adding over 2ms overhead every time we make a span :/ I think after we get this cleanup setup that some benchmarking is in order.

Makes sense although from what I quickly checked it's just first execution only, in iex first run for Spandex.Datadog.Span.new() takes ~4ms, next calls are around ~26-50 microseconds. Still we should check it out, I would say that at least publishing trace to agent should be made from a separate gen server with some retries asynchronously (as now it take place on trace finish as far as I understand)

coveralls commented 7 years ago

Coverage increased (+2.0%) to 44.444% when pulling 23cd446dd2142709d276e04b2ddcd68e8642cdd4 on driv3r:bugfix/7-use-config-for-defaults into 6fc831b7a90bcfcbbac0dddf136c9bb8f1d3146f on zachdaniel:master.

coveralls commented 7 years ago

Coverage increased (+2.1%) to 44.604% when pulling 00d9ac014b7500b1414657d63c9d01680492358c on driv3r:bugfix/7-use-config-for-defaults into 6fc831b7a90bcfcbbac0dddf136c9bb8f1d3146f on zachdaniel:master.

zachdaniel commented 7 years ago

I definitely agree about starting a trace sending server. I'll make an issue for it.

coveralls commented 7 years ago

Coverage increased (+18.4%) to 60.932% when pulling 56b10b84ccb1108a484ed3cf495b32c5479b224a on driv3r:bugfix/7-use-config-for-defaults into 6fc831b7a90bcfcbbac0dddf136c9bb8f1d3146f on zachdaniel:master.

driv3r commented 7 years ago

Ok, I think that's all for this PR, I actually should only update AddContext plug by removing entries which are already set in start_trace, but I also added tests and check if we actually trace stuff. Still a lot of tests to add.

I'll tackle #6 in separate PR after this one is merged

coveralls commented 7 years ago

Coverage increased (+18.4%) to 60.932% when pulling c9f901e66dedbecff94bc3a2901b08be087f2e94 on driv3r:bugfix/7-use-config-for-defaults into 6fc831b7a90bcfcbbac0dddf136c9bb8f1d3146f on zachdaniel:master.