New agent instrumentation for Apache Sling #9469
Conversation
|
@rombert could you also add tests for your instrumentation |
|
@rombert with cassandra there is no need to inject the agent into the image as we are testing cassandra client not the server. |
breedx-splk
left a comment
breedx-splk
left a comment
There was a problem hiding this comment.
Implementation looks good to me. I agree with @laurit tho and would like to see tests.
| name = servlet.getServletConfig().getServletName(); | ||
| } | ||
| if (name == null || name.isEmpty()) { | ||
| name = servlet.getServletInfo(); |
There was a problem hiding this comment.
I could see this as potentially surprising if it shows up as the "name", but I think it's fine. Hopefully more informative than just the class name when the name is missing from the config.
There was a problem hiding this comment.
I could've sworn that I found this pattern in the repository, but I can't find it anymore.
|
Thanks for the reviews @laurit and @breedx-splk . This is still on my to-do list but I did not manage to get back to it. Hopefully soon :-) |
|
@rombert are you still working on this? |
|
@jaydeluca - it's on my list, yes, but it gets pushed back by other priorities. I don't have a timeline for resuming work on it. |
|
I just rebased to the latest main branch. Still plan to look into adding tests. |
Add a new http-based test for Apache Sling based on the latest released Sling Started demo application. This commit shows how testing should work but it has a number of issues: - ignores advice failures - ignores muzzle failures - does not clean up the existing Sling instance - the code that starts up the Sling instance is fragile
|
🔧 The result from spotlessApply was committed to the PR branch. |
|
@breedx-splk @jaydeluca @laurit - I implemented an initial test, it has lots of hiccups though. Would be great to have some feedback before diving deeper:
On my side, the code to startup Sling needs to be improved and the code to shutdown needed to be added :-) We don't really have an 'embedding' use case so the APIs are lacking for now. Thanks! |
|
Oh, and a follow-up question: our next release will be based on Jakarta Servlet instead so the instrumentation will no longer work with it. Should I already move the current instrumentation to a different folder in preparation for two co-existing instrumentations ? I expect the logic to be the same and the tests identical but the APIs will be different so I don't see this working easily inside a single project. I would call the folder |
It is hard to say anything because the tests don't really work as they are. I think the problem is that the
I think that currently this isn't too important as I'd expect it to be not too hard to get it working with jakarta when you have it already working with javax. You can start with implementing it for javax or jakarta whichever is more comfortable for you and add the other later. |
4 participants
The new Apache Sling instrumentation ensures that top-level routes reflect the Sling Servlet (or script) handling the script and also that nested spans created by including components point to the right handler.
I've done extensive testing locally but since this is my first contribution there might be some rough edges. I'll add some screenshots to clarify what this looks like in practice.
Rendering with nested components
Rendering with top-level servlet