Analysis of instrumentation customisation

[iNikem]

Related to #566, #1622 and #778

I am going to summarise my current understanding of various options for overriding any given instrumentation in custom vendor distributions. The goal is not to arrive at an ideal solution, but at a solution good enough for GA. I focus on the following motivation for such customisation:

Instrumentation X from Otel distribution creates span that I don't like and I want to change it in my vendor distro.

To simplify the following discussion, let me take as an example some database client instrumentation that creates a span for database call and extracts data from db connection to provide attributes for that span.

I don't want this span at all

The easiest case. You can just pre-configure your distribution and disable given instrumentation.

I want to add/modify some attributes and their values does NOT depend on a specific db connection instance.

E.g. you want to add some data from call stack as span attribute. In this case just provide your custom SpanProcessor. No need for touching instrumentation itself.

I want to add/modify some attributes and their values depend on a specific db connection instance.

Write a new instrumentation, which injects its own advice into the same method as the original instrumentation. Use getOrder method to ensure it is run after the original instrumentation. Now you can augment current span with new information.

I want to remove some attributes

Write custom exporter or use attribute filtering functionality in Collector

I don't like Otel span at all. I want to significantly modify it and its lifecycle

Disable existing instrumentation. Write a new one, which injects Advice into the same (or better) method as the original instrumentation. Write your own Advice for this. Use existing Tracer directly or extend it. As you have your own Advice, you can control which Tracer you use.

======

The analysis above has some assumptions built-in. First, it assumes that custom instrumentation writer has access to the source code of the original instrumentation. They will have to copy-paste code from transformers() method of the original instrumentation. This means, among other things, that changes in the original instrumentation's transformers method may be breaking changes for custom instrumentation.

Second, it assumes that Otel TypeInstrumentations are sufficiently fine-grained. If an instrumentation has a large number of advices packed into it, it may be difficult to replace just one of them.

Also this analysis does not address the problem of changing a lot of similar instrumentations at once. E.g. "in all database client instrumentations add this extra attribute". This may be achieved by extending #778.

[iNikem]

@pavolloffay Reading this text above ^^, are you still sure that you want to extend existing instrumentation class in your vendor distribution?

[pavolloffay]

@pavolloffay Reading this text above ^^, are you still sure that you want to extend existing instrumentation class in your vendor distribution?

I wanted to extend the core instrumentation to avoid repeating the typeMatcher, classloaderMatcher, contextStore and be sure my custom instrumentation uses the same name as the core one (although my instrumentation should have also a separate name to separate on/off).

I want to remove some attributes

This is interesting and I think that there are benefits of removing attributes directly in the user app process to keep the volume of data low as early as possible.

From the gitter channel:

• the custom instrumenations should have unit tests therefore publishing core instrumentation maven artifacts is essential. * The instrumentation modules are also useful when custom instrumentation uses typed tracer e.g. Servlet2HttpServerTracer (use case: "I don't like Otel span at all. I want to significantly modify it and its lifecycle
  ").
• the custom build of the agent also uses javaagent-{tooling, bootstrap, api} artifacts and instrumentation-api

[iNikem]

I wanted to extend the core instrumentation to avoid repeating the typeMatcher, classloaderMatcher, contextStore and be sure my custom instrumentation uses the same name as the core one (although my instrumentation should have also a separate name to separate on/off).

For us to be on the same page: these are nice-to-have, but not required, right? Matcher methods are pretty small. And there is a trade-off between writing less code and have the danger of upstream instrumentation changes break your own instrumentation.

[pavolloffay]

For us to be on the same page: these are nice-to-have, but not required, right?

You are right, it's fine to copy them over, on the new release there is a unit test to verify that it works anyways. Although some newly added matches to the core might be missed and not copied over.

[pavolloffay]

I have just updated to Java agent 0.10.0 in my custom build hypertrace/javaagent#120.

In my custom servlet instrumentation I had to use servlet specific tracers to access the current span the Span.current() was throwing this error in unit tests, it would maybe also fail in the smoketest:

java.lang.VerifyError: (class: javax/servlet/http/HttpServlet, method: service signature: (Ljavax/servlet/ServletRequest;Ljavax/servlet/ServletResponse;)V) Illegal type in constant pool
	at java.lang.Class.getDeclaredConstructors0(Native Method)
	at java.lang.Class.privateGetDeclaredConstructors(Class.java:2671)
	at java.lang.Class.getConstructor0(Class.java:3075)
	at java.lang.Class.newInstance(Class.java:412)
	at org.eclipse.jetty.servlet.ServletContextHandler$Context.createServlet(ServletContextHandler.java:610)
	at org.eclipse.jetty.servlet.ServletHolder.newInstance(ServletHolder.java:760)
	at org.eclipse.jetty.servlet.ServletHolder.initServlet(ServletHolder.java:435)
	at org.eclipse.jetty.servlet.ServletHolder.getServlet(ServletHolder.java:336)
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:524)
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:480)
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:941)
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:409)
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:875)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:117)
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:110)
	at org.eclipse.jetty.server.Server.handle(Server.java:349)
	at org.eclipse.jetty.server.HttpConnection.handleRequest(HttpConnection.java:441)
	at org.eclipse.jetty.server.HttpConnection$RequestHandler.content(HttpConnection.java:936)
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:801)
	at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:224)
	at org.eclipse.jetty.server.AsyncHttpConnection.handle(AsyncHttpConnection.java:51)
	at org.eclipse.jetty.io.nio.SelectChannelEndPoint.handle(SelectChannelEndPoint.java:586)
	at org.eclipse.jetty.io.nio.SelectChannelEndPoint$1.run(SelectChannelEndPoint.java:44)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:598)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:533)
	at java.lang.Thread.run(Thread.java:748)

[trask]

In my custom servlet instrumentation I had to use servlet specific tracers to access the current span the Span.current() was throwing this error in unit tests, it would maybe also fail in the smoketest:

java.lang.VerifyError: (class: javax/servlet/http/HttpServlet, method: service signature:
(Ljavax/servlet/ServletRequest;Ljavax/servlet/ServletResponse;)V) Illegal type in constant pool

for others following here, this issue is continued over in #1661 (comment)

[pavolloffay]

Another use-case: extending instrumentation modules: #1677