spandex-project / spandex

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

Span meta leaking to children #50

Closed aspett closed 6 years ago

aspett commented 6 years ago

Due to https://github.com/zachdaniel/spandex/blob/master/lib/datadog/span.ex#L116, if you set meta on any span with Tracer.update_span, all children will also receive the meta. Is this intended?

zachdaniel commented 6 years ago

I'm currently redesigning those structs, but in the current implementation the same thing would happen. I would say it was intended, but not that its necessarily the correct thing. Certain things, like the service, resource, and type, should inherit from the parent span. I think there are probably other meta's that people might use, like user that can be set, and should probably inherit.

zachdaniel commented 6 years ago

By redesigning, I mostly just mean simplifying and making them not datadog specific. They still look mostly the same, but with a bit more structure.

zachdaniel commented 6 years ago

What do you think? Do you think it should be configurable what does/doesn't inherit from the parent span? Or that only meta should not inherit? Or somewhere in between?

aspett commented 6 years ago

I was expecting to use it to attach some extra data to a single span, but I totally understand you'd want it to inherit in some cases. Perhaps there needs to be a way to do both

zachdaniel commented 6 years ago

I think that would make sense. We could have a separate section for span meta that should and shouldn't inherit? The interface for this will be significantly cleaner (and more documented) when I'm done with updating the interface, and it would be easier than it is now to add that sort of thing. Very easy, actually. Something like an uninherited_meta. In the new design, you specify where things go explicitly (and use keywords), so it looks like this:

update_span(http: [url: url], error: [exception: Exception], meta: [some: 1, meta: 2])

So adding some kind of private_meta: [some: 1] or w/e we want to call it becomes a lot easier.

aspett commented 6 years ago

Sounds good