Add error parameter to EndTimeExtractor and AttributesExtractor#onEnd()

[mateuszrzeszutek]

I've added a @Nullable Throwable error parameter to EndTimeExtractor and AttributesExtractor#onEnd(). Now, along with the SpanStatusExtractor, every method in the Instrumenter API that accepts a RESPONSE also accepts a Throwable. The operation can end with either a RESPONSE or an error being passed to the instrumenter so I thought it makes sense to always pass both of these to have a complete view of how it ended.

This PR resolves #3730. It is now possible to extract custom attributes in an AttributesExtractor and they'll be passed along to the RequestListener, thus making it possible to use those attributes in custom metrics.

[trask]

wondering a little bit about passing original error vs passing the extracted error here.

do you think ErrorCauseExtractor might be used/useful for suppressing "expected" exceptions from being recorded? (and maybe still wanting to add a tag that there was an error, just wanting to suppress the big stack trace from getting recorded?)

or maybe there's a better mechanism for this use case

[mateuszrzeszutek]

do you think ErrorCauseExtractor might be used/useful for suppressing "expected" exceptions from being recorded?

I think that'd be a misuse of the API. It was originally meant to strip out those "meaningless" exceptions that wrap over the "real" ones, like ExecutionException.

The stacktrace issue seems to be a real one though, we have #431 in our backlog. Perhaps we could think about an additional configuration knob on the InstrumenterBuilder for that.

[trask]

ya, that makes sense to me 👍

[zmapleshine]

Hey @mateuszrzeszutek ,thank you for resolve the problem in #3730 .But...I do not understand the question about the label value of a metrics.
Because from the design of Instrumentation API module, it is almost certain that the label value of a metrics comes from attributes, but it is a "processed" value, and sometimes such a value from attributes is not suitable as label of a metrics . Similarly, labels that were put into attributes for use in metrics do not have to be a span tag .
I think if it is possible to pass the original object to the hook function as well? Or is there a clever way to combine trace and metrics organically without coupling?

[zmapleshine]

but it is a "processed" value, and sometimes such a value from attributes is not suitable as label of a metrics . Similarly, labels that were put into attributes for use in metrics do not have to be a span tag .

In a practical scenario, in the monitoring process of the Lettuce client, commands and parameters are usually concatenated together as span labels:

sql.statement: mget key1,key2...

Then, attributes must have a key named "sql.statement", but this value is not suitable for a label of redis metric, because the cardinality is too large and "mget" should be used instead.

[trask]

@mateuszrzeszutek this one has merge conflict

[mateuszrzeszutek]

Because from the design of Instrumentation API module, it is almost certain that the label value of a metrics comes from attributes, but it is a "processed" value, and sometimes such a value from attributes is not suitable as label of a metrics . Similarly, labels that were put into attributes for use in metrics do not have to be a span tag .

Yeah that's true. In case of metrics, the View API should solve that problem, but we don't really have anything like that for spans (well, you can remove attributes in the collector I guess).

I think if it is possible to pass the original object to the hook function as well?

I wanted to avoid passing the response & exception everywhere. I think that those endAttributes passed to the RequestListener are basically a derivative of the response, they should contain all useful information. Passing a response object to the listener would duplicate some of the AttributesExtractor logic and responsibility. I'd like to get @anuraaga's opinion on that though - whether what I'm describing here makes sense or not.

In a practical scenario, in the monitoring process of the Lettuce client, commands and parameters are usually concatenated together as span labels:

sql.statement: mget key1,key2...

Then, attributes must have a key named "sql.statement", but this value is not suitable for a label of redis metric, because the cardinality is too large and "mget" should be used instead.

That's why almost all (if not all) database clients' instrumentations emit db.operation attribute - in case of your example it'd contain just "mget" and would be a perfect choice for a metric label/attribute.

[zmapleshine]

That's why almost all (if not all) database clients' instrumentations emit db.operation attribute - in case of your example it'd contain just "mget" and would be a perfect choice for a metric label/attribute.

@mateuszrzeszutek
So how do you extract the URI from the attributes of HTTP client instrumentation? : ) :) :)

It might be given a full URL like "http://10.x.x.x/user/123456?sign=xxxxx&...", but it's not appropriate for metrics, which typically require a tag like "/user/{id}" (perhaps splicing together the service name to buid a new tag called "router", perhaps a slightly inappropriate example, but I mean the loss of the original request object may also lose the accuracy of the metric tag and even increase the difficulty of obtaining the tag value.)

[anuraaga]

The goal of instrumentation is to collect data - how the data gets transformed aside from that is signal specific. Traces generally follow a pattern of full data on a sample of traces. So spans just accept the attributes as is.

Metrics instead don't sample but aggregate - the metric view configuration configures the aggregation. Just like they aggregate points to a sum, the views will also be used to pick correct attributes - attribute selection is a sort of aggregation after all. The intention is definitely not to record high cardinality attributes. But the metrics SDK is currently in its infancy so we are still working on that piece of machinery. Hopefully that makes sense.

[anuraaga]

Mapping error to semantic attributes makes a lot of sense.

Less sure about end time ;) I guess it's symmetry but don't expect that to ever be useful.

[mateuszrzeszutek]

Less sure about end time ;) I guess it's symmetry but don't expect that to ever be useful.

Yeah, I added it there mostly for symmetry - honestly I have no idea how you can extract a useful end time from an exception.

[trask]

Less sure about end time ;) I guess it's symmetry but don't expect that to ever be useful.

Yeah, I added it there mostly for symmetry - honestly I have no idea how you can extract a useful end time from an exception.

is the symmetry "everywhere we pass RESPONSE, we also pass error"?

[mateuszrzeszutek]

is the symmetry "everywhere we pass RESPONSE, we also pass error"?

Yep, exactly that.