search

spandex-project / spandex

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

Add missing trace_id to logger metadata #107

Closed dnlserrano closed 4 years ago

dnlserrano commented 4 years ago

Does it make sense to log the trace_id in these cases as well? Thanks.

zachdaniel commented 4 years ago

At first glance it would seem to me like we just need to set the metadata in do_start_span. Do your tests pass if you remove the metadata from being set in the macros?

dnlserrano commented 4 years ago

Yap, they do. Updated. I didn't remove the Logger.metadata(span_id: span_id) that was in the decoration code anyway. I'm afraid that might introduce some unexpected changes. Better keep it? Thanks.

dnlserrano commented 4 years ago

Maybe I'm being over-protective of breaking changes and we can just remove it and add it back in case of issues (logging wouldn't be a major issue anyway, I don't think). So as long as tracing still works, which wouldn't change if I remove that. Let me know, I can definitely do it.

zachdaniel commented 4 years ago

Honestly, looking at the code, it looks like we'd rather not do that span context in the macro. Primarily because they all eventually call into the do_start_trace or do_start_span functions, which set metadata only if the trace is active. Having it in the macro could result in seeing a span_id for a span that was never actually created.

zachdaniel commented 4 years ago

So seems like moving it all into the do_start_* functions and leaving it out of the macro would be the way to go.

dnlserrano commented 4 years ago

Updated, tests still pass. Check commit message: 99f043e.