Sort instrumentations before adding them to ByteBuddy

[pavolloffay]

Signed-off-by: Pavol Loffay p.loffay@gmail.com

Use case: I am adding custom instrumenations that depend on the existing instrumentation from the core (for instance to get the current span). I want to make sure that the existing instrumentation is added to ByteBuddy before my custom instrumentation.

[iNikem]

How exactly do you think achieving the desired order? Can you give an example?

[pavolloffay]

How exactly do you think achieving the desired order? Can you give an example?

Let's say upstream instrumentation is named A.class the dependent custom instrumentation will have name AZ.class

BTW. I have noticed that the instrumentations are always loaded in alphanumeric order, however the java service loader does not document that it's guaranteed so I would like to sort them before adding to BB.

[iNikem]

You mean, e.g. JMSMessageListenerInstrumentationZ.class ? Seems like very hacky :( Although I have to admit, I don't have a better option right away, I am not sure this is viable long term solution

[pavolloffay]

Seems like very hacky :( Although I have to admit, I don't have a better option right away, I am not sure this is viable long term solution

At least it solves my problem :) It is clearly described that it's better to combine advice methods, however this is not always possible. The custom instrumentations are tested with the upstream version so the bugs should be easily detected.

An alternative approach could be to expose getPriority() in instrumentations and sort based on that. That said this is currently a blocker for me, could we move it forward?

[iNikem]

@trask @tylerbenson WDYT?

[trask]

An alternative approach could be to expose getPriority() in instrumentations and sort based on that

I like this, glowroot has similar:

https://github.com/glowroot/instrumentation/blob/0a70212737947f40b0cc90d90016a6b5db8f8b95/instrumentation-api/src/main/java/org/glowroot/instrumentation/api/weaving/Advice.java#L105-L120

I like the term "order" over "priority" because the term priority is always unclear to me whether low or high values have preference (thanks linux 😂). But I can live with priority if others prefer that.

[tylerbenson]

I also don't like the "alphabetical sort" idea. I don't have strong feelings between order/priority.

[iNikem]

+1 for Order

[pavolloffay]

Alright let's go with the order

[pavolloffay]

@iNikem @trask I have updated the PR and added getOrder to instrumenter interface.

[anuraaga]

Cool

[iNikem]

So instrumentations with bigger order will override those with smaller order?

[pavolloffay]

So instrumentations with bigger order will override those with smaller order?

Nothing is overridden. All advice methods will be applied. However the advice method that runs as second has access to the current span from the previous advice method.

[iNikem]

Right, we are not talking about overriding here, we are talking about the order of application.

[mateuszrzeszutek]

So instrumentation with order=1 runs after another one that has order=0, right? Could you include this in the JavaDoc?

[pavolloffay]

@mateuszrzeszutek I have added to it to the Javadoc.

[trask]

👍