Port Spring Boot WithSpanAspect to Instrumenter API

[HaloFour]

Port spring-boot-autoconfigure project WithSpanAspect and supporting annotation types to the newer Instrumenter<REQUEST, RESPONSE> API.

Adds some new classes to instrumentation-api-annotation-support to fit with the Instrumenter<REQUEST, RESPONSE> API. These new classes are shims to the existing types. I plan on following this PR up with similar changes to the opentelemetry-annotations-1.0 project at which time I will refactor the logic from the shimmed classes into the new classes.

[anuraaga]

I think the general approach of an "annotations instrumentation API" makes sense since we have several of them. I guess there isn't any real win from the actual Instrumenter API (or BaseTracer for that matter) here since there aren't any semantic conventions - annotations seem to be a annotation-driven wrapper around the OpenTelemetry API itself, not "real instrumentation" for lack of a better word. @HaloFour Do you think it's helping or hurting to use Instrumenter here as opposed to just sticking with direct usage of the OpenTelemetry API?

[HaloFour]

I don't think it hurts using Instrumenter API here. I was a little skeptical at first that the API (as it exists) wouldn't have the necessary knobs but that concern seems unfounded, and the class wraps most of the other common concerns well enough.

I've been working on the opentelemetry-annotations-1.0 project in tandem with this one and where the two have differences this API has worked well and I think the resulting code is at least marginally simpler.

Is the intention to eventually deprecate/remove BaseTracer?

[mateuszrzeszutek]

cache is now always non null

[mateuszrzeszutek]

Nit:

Suggested change

	public class MethodSpanAttributesExtractor<REQUEST, RESPONSE>
	public final class MethodSpanAttributesExtractor<REQUEST, RESPONSE>

[mateuszrzeszutek]

parameterAttributeNamesExtractor seems to be optional in the builder. What if it's not set? Should it be required?

[mateuszrzeszutek]

Nit: the class itself is package-private?

Suggested change

	public JoinPointRequest(JoinPoint joinPoint) {
	this.joinPoint = joinPoint;
	MethodSignature methodSignature = (MethodSignature) joinPoint.getSignature();
	this.method = methodSignature.getMethod();
	this.annotation = this.method.getDeclaredAnnotation(WithSpan.class);
	}
	public Method method() {
	return method;
	}
	public WithSpan annotation() {
	return annotation;
	}
	public Object[] args() {
	return joinPoint.getArgs();
	}
	JoinPointRequest(JoinPoint joinPoint) {
	this.joinPoint = joinPoint;
	MethodSignature methodSignature = (MethodSignature) joinPoint.getSignature();
	this.method = methodSignature.getMethod();
	this.annotation = this.method.getDeclaredAnnotation(WithSpan.class);
	}
	Method method() {
	return method;
	}
	WithSpan annotation() {
	return annotation;
	}
	Object[] args() {
	return joinPoint.getArgs();
	}

[mateuszrzeszutek]

Is the intention to eventually deprecate/remove BaseTracer?

Yep, we intend to do that once it's no longer used anywhere.

[laurit]

Our weak caches use identity comparison for keys. For this cache to work it would need to get the exact same method instance. For example Class.getDeclaredMethod returns a different instance each time. Are you sure that MethodExtractor implementations will actually always return the same instance? Should this be documented? Or use a different caching strategy, for example could use a ClassValue on declaring class of the method that contains a plain concurrent map. If you believe that the cache should be weak then you could use ClassValue to hold keys that you derive from method so that different instances of the same method would have the same key.

[HaloFour]

Oof, I did not know that our cache implementation was limited to identity comparison of weak keys. I see that this behavior is effectively inherited from Caffeine so we don't have an easy way to support value-based equality of weak keys.

I ran some tests locally and it seems like Spring AOP does cache the Method instance on subsequent invocations, but I am a little hesitant to depend on this behavior.

[HaloFour]

Unfortunately ByteBuddy does not cache the Method instance. When referencing @Advice.Origin Method method it effectively emits Foo.class.getMethod("name", ...) on each invocation. I'll look into your suggestions on different cache strategies.

[HaloFour]

I added an implementation of Cache<K, V> that uses ClassValue<T> under the hood. It's probably overkill to have it implement Cache<K, V> since I'll only ever use computeIfAbsent here. The use of ConcurrentHashMap is probably also overkill but very rudimentary testing seems to indicate that it performs better for this read-mostly scenario even with relatively few entries.

[mateuszrzeszutek]

Hmm, since all 3 params are required, WDYT about using a static factory method instead of a builder?

[HaloFour]

Good idea. I was trying to make it fit into the same builder pattern as Instrumenter but I agree that it's not a good fit.

[trask]

thx 👍