maven icon indicating copy to clipboard operation
maven copied to clipboard

MNG-7479 - Export the package org.apache.maven.eventspy

Open laeubi opened this issue 2 years ago • 12 comments

I'm currently try to fetch a specific EventSpy to inject an event there (maven-profiler).

Sadly in my maven plugin when I try to get it injected:

@Requirement(role = EventSpy.class, hint = "profiler", optional = true) EventSpy eventSpy;

This results in ClassNotFoundException: org.apache.maven.eventspy.EventSpy

I noticed that the maven-core do not export the 'org.apache.maven.eventspy' package and that is the cause of this, I'd like to suggest to export the package as there seem no other standard way to interact/access event spys otherwise.

Following this checklist to help us incorporate your contribution quickly and easily:

  • [x] Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • [x] Each commit in the pull request should have a meaningful subject line and body.
  • [x] Format the pull request title like [MNG-XXX] SUMMARY, where you replace MNG-XXX and SUMMARY with the appropriate JIRA issue. Best practice is to use the JIRA issue title in the pull request title and in the first line of the commit message.
  • [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • [x] Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • [ ] You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004 you have to acknowledge this by using the following check-box.

laeubi avatar May 13 '22 08:05 laeubi

This currently blocks:

  • https://github.com/eclipse/tycho/issues/187

Related to:

  • https://github.com/jcgay/maven-profiler/issues/125

laeubi avatar May 13 '22 08:05 laeubi

@michael-o do you think this could make it into the upcoming 3.8.6 release? That would be very helpful and should be easily cherry picket to 3.9.x and 4.x as well...

laeubi avatar May 13 '22 08:05 laeubi

EventSpy is not part of plugin API and is hidden for good reasons, it is for maven extensions as Javadoc of EventSpy nicely explains. Event spy emits Maven Core events (about processing the build, among them mojos as well) to some external listener extension.

For profiling please look into these

  • https://github.com/takari/maven-profiler
  • https://github.com/jcgay/maven-profiler

If you want to "benchmark" Mojo internals, why do you need event spy at all? Event spy is not at all meant for this.

-1

cstamas avatar May 13 '22 11:05 cstamas

The event spy is the only one capable of producing actual timing for mojos. We have brought one inspired from takari in mvnd iirc : https://github.com/apache/maven-mvnd/blob/master/daemon/src/main/java/org/mvndaemon/mvnd/timing/BuildTimeEventSpy.java That's a perfectly valid use case imho.

@laeubi if you need to interact with your specific event spy, you could inject the implementation or another interface instead of the EventSpy interface which is meant to be actually used by maven and does not provide any information. Or do I miss something ?

gnodet avatar May 13 '22 13:05 gnodet

@cstamas I don't think the javadoc is that strict as you try to depict here. It only say that EventSpy is a core extension but why should core extension interfaces not be accessible? Beside that, I even don't see a reason why not an plugin-extension should be able to receive events, but that's another topic ..

About your links, if you take a closer look at this PR you will see I have linked exactly your second ref here, sorry if it was not clear enough (I updated my comment).

Event spy emits Maven Core events (about processing the build, among them mojos as well) to some external listener extension.

Not sure if we are talking about the same thing, actually EventSpy is the "some external listener extension" that receives events, the events are emitted by EventSpyDispatcher what resides in an internal package and I don't propose here to export that package...

If you want to "benchmark" Mojo internals, why do you need event spy at all?

I want to look up one specific event spy to inject additional events to that listener so they appear in the report of that plugin as if they where emitted by maven, maybe I even later on want to emit custom ones, the reason for this is simple, I don't want another implementation or report but reuse/extend an existing one.

if you need to interact with your specific event spy, you could inject the implementation or another interface instead of the EventSpy interface which is meant to be actually used by maven and does not provide any information. Or do I miss something ?

As the implementation is not controlled by me, the idea was just to have as less dependencies to it as possible, and for that I can e.g. send a RepositoryEvent to the EventSpy#onEvent, so just assume some time later on the implementation was renamed/moved I need not adjust my code (the implementer even might consider its implementation as internal, what is perfectly valid). I think that's the nice thing about the container look-up that I don't need to bind to the specific implementation but could choose by the role-hint...

laeubi avatar May 14 '22 04:05 laeubi

By the way I can always inject me the container

@Requirement
PlexusContainer container;

and then lookup the object by its (string) class name

Object eventSpy;
try {
    eventSpy = container.lookup("org.apache.maven.eventspy.EventSpy", "profiler");
    System.out.println("EventSpy is " + eventSpy);
} catch (ComponentLookupException e) {
    // TODO Auto-generated catch block
    e.printStackTrace();
}

and then use reflection to call the EventSpy#onEvent, but this really makes the code quite ugly and only because I can't load the interface itself...

laeubi avatar May 14 '22 05:05 laeubi

how do you expect filtering to happen to know which instance of EventSpy is expected in your Mojo instance? There are thousands of EvenSpy instances, even when you filter only a certain type

definitely, the lifecycle of EventSpy objects seems completely not adapted to using IoC injection to me

Perhaps this study about LifecycleParticipant could help you https://maven.apache.org/studies/extension-demo/

hboutemy avatar May 14 '22 07:05 hboutemy

how do you expect filtering to happen to know which instance of EventSpy is expected in your Mojo instance?

I don't have a mojo here, just o (project scoped) extension, according to the code I won't expect thousands but one instance per session?

Perhaps this study about LifecycleParticipant could help you

Not really, just in case I accesses the Session and the Execution request, but calling:

MavenExecutionRequest request = mavenSession.getRequest();
EventSpyDispatcher dispatcher = request.getEventSpyDispatcher();

results in ClassNotFoundException: org.apache.maven.eventspy.internal.EventSpyDispatcher... so no luck here.

laeubi avatar May 14 '22 07:05 laeubi

oh, I'm starting to understand:

  1. you want to inject custom events
  2. and these events will be usable from an EventSpy that knows about such custom events

that's it?

hboutemy avatar May 14 '22 12:05 hboutemy

Yes custom ones but also standard ones, in the tycho-plugin we also have a repository (that has nothing to do with the maven repo) and my first step would be to issue repository download start/end events so they occur in the maven-profiler output.

laeubi avatar May 14 '22 14:05 laeubi

I see making EventSpy visible in plugins is not the right approach: it would bypass the whole events organisation instead, clarifying how to use EventDispatcher (I imagine)

in your context, is it from a plugin classloader or core classloader (extension or plugin marked as extension)? because until now, AFAIK, events are only dispatched from core: if we need to dispatch from another context, we'll have to check how it impacts the dispatch workflow

hboutemy avatar May 14 '22 17:05 hboutemy

it would bypass the whole events organisation instead

Just looking at the code, the organization is rather loose coupled, passing arbitrary objects and implementations have to check for what they get so it seems rather unlikely that this would harm anything here, and I think one don't gain very much (as I said I can lookup the component anyways and use reflection).

And it seems a bit strange that one has references to even internal EventDispatcher in a 'public' interface if the class it self can't be loaded.

in your context, is it from a plugin classloader or core classloader (extension or plugin marked as extension)?

plugin marked as extension

AFAIK, events are only dispatched from core: if we need to dispatch from another context, we'll have to check how it impacts the dispatch workflow

I think as soon as one injects own events he/she is responsible to not disturb the handling anyways and it does not make much of a difference if you are a plugin, plugin-extension or core-extension ...

laeubi avatar May 16 '22 04:05 laeubi

@gnodet can new API cover this use case?

cstamas avatar Oct 04 '22 18:10 cstamas

@gnodet can new API cover this use case?

There's an event / listener interface, but this is currently limited to execution events. We'd have to open it up a bit more to other kind of events.

gnodet avatar Oct 04 '22 20:10 gnodet

@gnodet would be great to have a solution for Maven4 so extensions can participate in producing events.

laeubi avatar Nov 29 '23 06:11 laeubi