Added Configuration.WithExpandExceptionLogs

[Falco20019]

Right now, when using Configuration, it's not possible to call WithExpandExceptionLogs() on the TraceBuilder without using GetTracerBuilder() and Build it directly instead of using GetTracer(). It seems, that this is nearly the only flag that can not be set by environment.

I assume this should be discussed and proposed in the meta repo? But I first wanted to hear if @cwe1ss or @yurishkuro already have an opionion on that.

[yurishkuro]

is there a situation where we'd want to not capture the stacktrace? Maybe it should be the default behavior without requiring additional configuration.

[Falco20019]

I assume this is mostly a compromise between having redundant data to search tags or only have all the information not searchable in error.object

How is this handled in Java?

[yurishkuro]

we don't have a well defined process for this at the moment in any client library. I was not thinking of making the stack traces searchable, but just to capture the stacktrace fully. Other tracing formats (e.g. x-ray and census) have a well defined structure in the span for capturing exceptions. We may want to extend the upcoming protobuf format with that as well.

[Falco20019]

@yurishkuro Just checked the java implementation and it's the same there. Default seems to be using error.object and copy the full exception/throwable and if WithExpandExceptionLogs() is called on the Tracer.Builder, it's decomposed into error.kind, message and stack. The stack is never decomposed any further, but I think that's fine for now.

So even in Java, it's currently not possible to set WithExpandExceptionLogs directly on Configuration without having to get the Tracer.Builder and call it on that instead of getting the Tracer directly from Configuration.

[Falco20019]

Maybe we could add this to v0.2.0 since we are already want to make changes to Configuration.

[Falco20019]

@yurishkuro This is the only configuration flag right now that is not settable through environment. We added JAEGER_TRACEID_128BIT yesterday, so maybe we could also add JAEGER_EXPAND_EXCEPTION_LOGS. At least Java also has the WithExpandExceptionLogs so it's not just a C# feature. As written above, default behaviour is to not expand to reduce redundant logs for the costs of it being harder filterable. Maybe also relates to jaegertracing/documentation#105

[yurishkuro]

what would be a situation when someone would not want to expand exceptions this way?

[Falco20019]

Already part of the Tracer.Builder API. So both ways are supported then

[yurishkuro]

I realize that, my question is on the broader scope of this being a standard env var across languages, do we have enough justification for this behavior to be configurable rather than "always expand"?

[Falco20019]

I assume it’s a legitimate decision to choose between non-redundant storage (not expanded) and searchability (expanded) and a decision the developer has to make. Searchability may also be limited even with expanded enabled since stack traces are usually language specific. So the only benefit would be the unified error.message

[yurishkuro]

If you look at opencensus (and probably x-ray) span data models, they have explicit data structure for exceptions and the stack trace.

I assume it’s a legitimate decision to choose between non-redundant storage (not expanded) and searchability (expanded)

Is it fair to rephrase this as a trade-off between expressiveness of the captured data and overhead of collecting & storing it (I'd argue mainly collecting since it's a client library). In this case, what concerns me with an explicit option for just a single aspect that affects this trade-off is where it ends. For example, I remember Lightstep tracer has an option on the max number of logs to keep in a span. Does the user actually care? If the only concern here is the performance trade-off, as a user I might simply want a single knob that I can turn from 0 to 1 for min overhead to max expressiveness. In other words, the user defines a certain overhead threshold, and it's up to the tracer to decide how it achieves that. Maybe one span has 100 logs and so there's no space for the stacktrace, while the other has a single log and a stracktrace would be perfectly fine to capture.

[Falco20019]

@yurishkuro Is this already solved in any of the other libraries?

[yurishkuro]

Is this already solved in any of the other libraries?

no

[Falco20019]

I will keep that for now that way since we should at least have the full API configurable. But I would add the discussion above into a separate issue for the future. I think you have a point that the user should not have to care about low-level flags but about behaviour. This will need some adjustments to the Tracer and maybe some other classes, which is definetly not the focus of this Configuration enhancement.

@cwe1ss @grounded042 Would be good if someone could have a look over it.

[Falco20019]

Not sure if we should do it using the default value. The idea was to keep it similar to Tracer.Builder.WithExpandExceptionLogs(), but also have it resettable.

[yurishkuro]

Could use a better description, it's not clear to me what this actually does.