tracing-forest icon indicating copy to clipboard operation
tracing-forest copied to clipboard

Implement SpanFieldEventLayer

Open rspencer01 opened this issue 3 years ago • 3 comments

This layer listens for updates to span fields and registers a TreeEvent with the span when this occurs.

This generates a TreeEvent rather than calling Context::event which is different to what was discussed in #20 . @QnnOkabayashi , if this is a satisfactory solution, please let me know, otherwise feel free to reject this PR and I'll go back to trying to work out how to create events with the "wrong" callsite, and getting a ValueSet out of Fields.

rspencer01 avatar Dec 04 '22 13:12 rspencer01

I'm not opposed to removing (not including) this from tracing-forest. What would be your thoughts on it being a separate crate, re-exported (with a feature flag) from tracing-forest?

(nonetheless, I've updated the PR for your thoughts on the various fixes)

rspencer01 avatar Dec 04 '22 18:12 rspencer01

After some thought, I think it would be a good idea to put it behind a feature flag like watch_span_fields.

My reasoning for this is that I realized after creating this library that there are a lot of cases where people (like you) might want to interface with the tree abstractions in a way that doesn't make sense to have built in to ForestLayer itself, and I think that adding extra Layers behind feature flags could be the way to go. For example, the fact that making an immediate = true field in an event prints it to stdout immediately should be behind a configurable extra Layer instead of in ForestLayer. So this could be a good first example of that.

If this seems okay, to you, we can create an extras module in the root and then a watch_span_fields module inside of that which is behind the feature flag.

QnnOkabayashi avatar Dec 05 '22 17:12 QnnOkabayashi

Although a more scalable solution would be my original idea, which is to make doing certain things on OpenedSpans and constructing Events part of a public API. Then these extra Layers could be their own crates or as part of the user's library, and we wouldn't need to make PR's for adding them. However, these nice APIs don't exist yet, so I would say we can merge this for now behind a feature flag as I said above. If we choose to make the public APIs, we can just do a breaking change and remove it from this crate if we feel necessary since this crate doesn't have a ton of users.

QnnOkabayashi avatar Dec 05 '22 17:12 QnnOkabayashi