Update HTTP metrics 'view' code to match the specification

[jsuereth]

Sending this for preliminary review/thoughts. Important callouts:

• There is a different set of rules for selecting identifying attributes for Client vs. Server.
• This set of rules tries to give you a series of attributes to reconstruct a "URL", for the most part. See the tests in TemporaryMetricsView for an example of what this does.
• The algorithm as it stands pays a runtime cost building the Set for the view every time it runs. Looking for ideas on how to optimise.

cc @trask

Things not done:

• The semantic conventions strongly encourage using "abstract URL" paths, e.g. routing rules to annotate URL vs. a specific URL. We could do things like "find /[\d]+/" and erase it to "id" automagically, perhaps. But I suspect this cardinality isn't quite so bad as the peer-ip address cardinality is right now.

[trask]

The algorithm as it stands pays a runtime cost building the Set for the view every time it runs. Looking for ideas on how to optimise.

We could create separate Sets for each "identity attribute" combination and then pass two Set<AttributeKey> params to applyView, e.g.

applyView(filtered, startAttributes, durationAlwaysInclude, identityInclude);

• The semantic conventions strongly encourage using "abstract URL" paths, e.g. routing rules to annotate URL vs. a specific URL. We could do things like "find /[\d]+/" and erase it to "id" automagically, perhaps. But I suspect this cardinality isn't quite so bad as the peer-ip address cardinality is right now.

I'm not sure how to make this work as a default configuration for the Java agent. So many application have very high-cardinality paths (and even more have very high-cardinality query strings).

I would opt for not including http.url or http.target by default, but also ok if you want to use this to get feedback from users since metrics is still alpha.

http.route would be a good alternative. We have lots of instrumentation to capture the low-cardinality route. We use it to populate the http server span name, though we don't (yet) populate the actual http.route attribute. We'd need to do some additional work to capture http.route in a way that will get that attribute passed to endAttributes (instead of simply calling Span.setAttribute mid-span), but that's something we can probably figure out.

[jsuereth]

I would opt for not including http.url or http.target by default, but also ok if you want to use this to get feedback from users since metrics is still alpha.

We need something like it to identify the real metric-stream that has latency issues.

http.route would be a good alternative. We have lots of instrumentation to capture the low-cardinality route. We use it to populate the http server span name, though we don't (yet) populate the actual http.route attribute. We'd need to do some additional work to capture http.route in a way that will get that attribute passed to endAttributes (instead of simply calling Span.setAttribute mid-span), but that's something we can probably figure out.

This sounds like what http.target is meant to be in the metric semantic conventions. I'd be all about moving to this, is that realistic to do?

I would opt for not including http.url or http.target by default, but also ok if you want to use this to get feedback from users since metrics is still alpha.

I wonder how much of the issue was actually around net.peer.ip vs. the url itself. I'd like to get rid of the pure url for something more path-y, but I think this metric isn't very usable without the path right now.

[trask]

This sounds like what http.target is meant to be in the metric semantic conventions. I'd be all about moving to this, is that realistic to do?

yes, I think so

I wonder how much of the issue was actually around net.peer.ip vs. the url itself. I'd like to get rid of the pure url for something more path-y, but I think this metric isn't very usable without the path right now.

Java web routing frameworks make it so easy to encode parameters into paths that it's really common.

Since metrics is alpha, we can merge as-is and open an issue to migrate to http.route. Let's give a day to see if feedback from anyone else.

[jsuereth]

Sounds good. Agreed that http.route is the right long-term solution. If it's there now, I'd be more than happy to cut this to:

http.scheme, http.host and http.route

if we consistently have them available in client + server.

[trask]

Sounds good. Agreed that http.route is the right long-term solution. If it's there now, I'd be more than happy to cut this to:

http.scheme, http.host and http.route

if we consistently have them available in client + server.

ah, that's a good point, there's very few Java http client libraries that have a "route" concept, so while http.route will be good for server instrumentation, we'd have to do something different for client instrumentation

[mateuszrzeszutek]

http.scheme, http.host and http.route

if we consistently have them available in client + server.

Unfortunately, none of our HTTP client instrumentations set these attributes; they're only set by the server isntrumentations (http.route is very rare though)

http.route would be a good alternative. We have lots of instrumentation to capture the low-cardinality route. We use it to populate the http server span name, though we don't (yet) populate the actual http.route attribute. We'd need to do some additional work to capture http.route in a way that will get that attribute passed to endAttributes (instead of simply calling Span.setAttribute mid-span), but that's something we can probably figure out.

I've been thinking of refactoring the ServerSpanNaming into a HttpRouteHolder (name's provisional 😄 ) that'll set both the span name and the http.route. I thought it could be added as a ContextCustomizer and retrieved from the context when needed to set the name/route. Passing this to the AttributesExtractor would probably require adding Context as an additional attribute, which would make end() accept 5 attributes -- which is quite a lot. Instead, since the RequestListener already gets the Context passed to its methods, maybe we could get the attribute from the context directly there?

[trask]

I've been thinking of refactoring the ServerSpanNaming into a HttpRouteHolder (name's provisional 😄 ) that'll set both the span name and the http.route.

👍

Passing this to the AttributesExtractor would probably require adding Context as an additional attribute, which would make end() accept 5 attributes -- which is quite a lot. Instead, since the RequestListener already gets the Context passed to its methods, maybe we could get the attribute from the context directly there?

this is going to be a problem for metrics for any attributes that get set mid-span. I wish we could call span.getAttributes() on end and use that as the baseline for metrics attributes. @jsuereth @jkwatson any ideas?

[jkwatson]

this is going to be a problem for metrics for any attributes that get set mid-span. I wish we could call span.getAttributes() on end and use that as the baseline for metrics attributes. @jsuereth @jkwatson any ideas?

No ideas here. Unfortunately, since we can't expose this data on the API's Span, due to the streaming SDK requirement, we're pretty hamstrung. It would have to be something that the SDK itself would be able to provide. Probably some new feature of combined tracing/logs/metrics SDK that would enable this kind of thing.

I've also been pondering that some of this could probably be accomplished if there were a way for an API user to register a "Span callback" that got updates to the span also fed to it, but I don't know what that callback interface would have to look like (probably almost exactly like the span interface?)

[jsuereth]

I've also been pondering that some of this could probably be accomplished if there were a way for an API user to register a "Span callback" that got updates to the span also fed to it, but I don't know what that callback interface would have to look like (probably almost exactly like the span interface?)

Would it make sense to have a span-processor that generates metrics? Really wish we had that level of integration.

However, this is all kind of beside the point.

What's the next steps on this PR? I think we need to clean up what we have now, as we know it causes issues. I've kind of lost track in this discussion what changes I should try to make.

[trask]

What's the next steps on this PR?

I'm not sure what's worse for the immediate 1.8.0 release 🤷‍♂️

• cardinality explosion due to inclusion of http.url/http.target, or
• useless metrics due to lack of http.url/http.target

I'm open to either. Let's chat and decide in tomorrow's SIG meeting(s).

[jsuereth]

Updated based on Java SiG discussions:

• We only use http.url on the client. I created a really dumb query-param scrubber that's meant to help limit cardinality. I can make this more robust or use an existing utility (ideally) if we have one.
• We use http.route on the server and fall back to http.target. This assumes we'll have cardinality limits in the SDK by the time this is released.

[trask]

thx!