Bug 57305 - remove ProxyControl dependency on GuiPackage
Summary: remove ProxyControl dependency on GuiPackage
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 3.0
Hardware: All All
: P2 enhancement with 2 votes (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2014-12-03 15:19 UTC by jarek102
Modified: 2016-11-03 09:58 UTC (History)
2 users (show)



Attachments
patch implementing this feature (4.56 KB, patch)
2014-12-03 15:19 UTC, jarek102
Details | Diff
Updated patch should apply clean to 3.0 source tree (5.11 KB, patch)
2016-10-15 01:02 UTC, Wyatt Epp
Details | Diff
"complete and working" example (1.60 KB, text/plain)
2016-10-18 17:12 UTC, Wyatt Epp
Details
Allow to use ProxyControl in non GUI mode (6.94 KB, patch)
2016-10-20 20:16 UTC, Felix Schumacher
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jarek102 2014-12-03 15:19:16 UTC
Created attachment 32256 [details]
patch implementing this feature

I'm running proxy without UI.

Main problem is that ProxyControl class uses 
GuiPackage.getInstance().getTreeModel() as source of tree model
to fix this i've added static member to ProxyControl, 
witch can be overridden by user.
Comment 1 jarek102 2014-12-03 15:24:43 UTC
Usage:

        treeModel = new JMeterTreeModel(new Object());
        
        ProxyControl.setGlobalTreeRoot(treeModel);
        
        JMeterTreeNode root = (JMeterTreeNode) treeModel.getRoot();
        treeModel.addSubTree(testPlanTree, root);
        
        proxy = new ProxyControl();
        proxy.setTarget(findTargetNode(root));
        proxy.setPort(8282);
        
        treeModel.addComponent(proxy, (JMeterTreeNode) root.getChildAt(1));
        
        proxy.startProxy();
Comment 2 Philippe Mouawad 2014-12-07 20:55:30 UTC
Hi,
Thanks for contribution.
To be complete it would be interesting to have an option to start proxy from command-line .
I see you start it through API manipulation.

Regards
Comment 3 Wyatt Epp 2016-04-08 17:04:36 UTC
Why is this NEEDINFO?  This is an important first step, and it would nice to get this merged.
Comment 4 Wyatt Epp 2016-10-15 00:58:20 UTC
Turns out this patch is actually important for being able to build against JMeter 3.0 and still make a valid JMeterTreeModel.

The method I was using for 2.13 was a shortcut that worked fine, but I had to rewrite it to create JMeterTreeNode.parent relations after updating.  But that caused this to become an issue at runtime.

The posted patch still applies mostly clean; needs two small changes.  I'll attach an updated version.
Comment 5 Wyatt Epp 2016-10-15 01:02:53 UTC
Created attachment 34374 [details]
Updated patch should apply clean to 3.0 source tree
Comment 6 Philippe Mouawad 2016-10-15 11:29:07 UTC
(In reply to Wyatt Epp from comment #5)
> Created attachment 34374 [details]
> Updated patch should apply clean to 3.0 source tree

Thanks for patch.
Could you give an example of how you use it ?
Thank you
Comment 7 Wyatt Epp 2016-10-17 13:03:51 UTC
(In reply to Philippe Mouawad from comment #6)
> (In reply to Wyatt Epp from comment #5)
> > Created attachment 34374 [details]
> > Updated patch should apply clean to 3.0 source tree
> 
> Thanks for patch.
> Could you give an example of how you use it ?

It's just an update of Jarek's patch, so usage is unchanged from Comment #2 (only necessary when you're running headless).
Comment 8 Philippe Mouawad 2016-10-17 13:43:40 UTC
My question was more, do you have a complete example of using it ?
For me it means you're using it through API.
Regards
Comment 9 Wyatt Epp 2016-10-17 13:59:27 UTC
(In reply to Philippe Mouawad from comment #8)
> My question was more, do you have a complete example of using it ?

Complete example of using... what, exactly?  It's not clear what you're looking for.

> For me it means you're using it through API.

Well... yes?  I mean, that's the only way to record headless, right now.

BTW, would it be asking too much to get this patch added to the 3.1 branch?
Comment 10 Philippe Mouawad 2016-10-17 14:07:04 UTC
(In reply to Wyatt Epp from comment #9)
> (In reply to Philippe Mouawad from comment #8)
> > My question was more, do you have a complete example of using it ?
> 
> Complete example of using... what, exactly?  It's not clear what you're
> looking for.

Of using JMeter in headless mode to record.
> 
> > For me it means you're using it through API.
> 
> Well... yes?  I mean, that's the only way to record headless, right now.

So my request is to have an example of using the API.
> 
> BTW, would it be asking too much to get this patch added to the 3.1 branch?

Yes if you are able to provide this example :-) 
Thanks
Comment 11 Wyatt Epp 2016-10-17 14:31:09 UTC
(In reply to Philippe Mouawad from comment #10)
> (In reply to Wyatt Epp from comment #9)
> > Complete example of using... what, exactly?  It's not clear what you're
> > looking for.
> 
> Of using JMeter in headless mode to record.

Please clarify your definition of "complete", then?

Anyway, the sample in Comment #2 should be plenty for anyone deep enough into the code that they're legitimately affected by this bug (especially prior to 3.0, where you didn't have to properly establish JMeterTreeNode.parent relations on your RecordingController et al to capture properly).
Comment 12 Philippe Mouawad 2016-10-17 14:45:15 UTC
(In reply to Wyatt Epp from comment #11)
> (In reply to Philippe Mouawad from comment #10)
> > (In reply to Wyatt Epp from comment #9)
> > > Complete example of using... what, exactly?  It's not clear what you're
> > > looking for.
> > 
> > Of using JMeter in headless mode to record.
> 
> Please clarify your definition of "complete", then?
> 
> Anyway, the sample in Comment #2 should be plenty for anyone deep enough
> into the code that they're legitimately affected by this bug (especially
> prior to 3.0, where you didn't have to properly establish
> JMeterTreeNode.parent relations on your RecordingController et al to capture
> properly).

If snipped above is fine, then that's what I mean by complete :-)
Comment 13 Wyatt Epp 2016-10-17 15:21:55 UTC
(In reply to Philippe Mouawad from comment #12)
> If snipped above is fine, then that's what I mean by complete :-)

Well, it was more than enough for me, so great!  It'll be very nice to have this; I had to reopen a bug in our internal tracker when I rolled back to 2.13.  Thanks!
Comment 14 Philippe Mouawad 2016-10-17 20:44:11 UTC
(In reply to Wyatt Epp from comment #13)
> (In reply to Philippe Mouawad from comment #12)
> > If snipped above is fine, then that's what I mean by complete :-)
> 
> Well, it was more than enough for me, so great!  It'll be very nice to have
> this; I had to reopen a bug in our internal tracker when I rolled back to
> 2.13.  Thanks!

Hi,
I used the sample above and it is not complete.
1)testPlanTree variable is not initialized
2) findTargetNode does not exist

If we add this patch to core, we need to provide a complete and working example and in this case see which API should be made public. That's why I am asking for those information.

Thanks
Regards
Comment 15 Wyatt Epp 2016-10-18 17:12:07 UTC
Created attachment 34386 [details]
"complete and working" example

(In reply to Philippe Mouawad from comment #14)
> I used the sample above and it is not complete.

What, you didn't expect that to run as-is, did you?

> If we add this patch to core, we need to provide a complete and working
> example

Oh, moving the goalposts on me? Rude.

I'll play along, but only because we actually need this patch merged so we don't have to maintain our own version of JMeter internally.

> and in this case see which API should be made public.

What?  No.  Stop.  The method this patch adds must be public; anything else would be *completely* useless.
Comment 16 Philippe Mouawad 2016-10-18 18:23:08 UTC
(In reply to Wyatt Epp from comment #15)
> Created attachment 34386 [details]
> "complete and working" example
> 
> (In reply to Philippe Mouawad from comment #14)
> > I used the sample above and it is not complete.
> 
> What, you didn't expect that to run as-is, did you?

I did :-)
> 
> > If we add this patch to core, we need to provide a complete and working
> > example
> 
> Oh, moving the goalposts on me? Rude.

:-) 
> 
> I'll play along, but only because we actually need this patch merged so we
> don't have to maintain our own version of JMeter internally.
> 
> > and in this case see which API should be made public.
> 
> What?  No.  Stop.  The method this patch adds must be public; anything else
> would be *completely* useless.

Of course for this one.

Thanks for patch and example
Comment 17 Philippe Mouawad 2016-10-18 20:55:56 UTC
Hello again @Wyatt,

Few questions:

- How is user supposed to use the Proxy. He starts it in non gui mode. But how does he saves the Test plan recorded ?

I suppose you have some custom development to do that ? Any chance you contribute more ?


- Would it make sense in the future to have a command line option to start proxy in NON GUI mode


Regards
Comment 18 Wyatt Epp 2016-10-18 22:09:32 UTC
(In reply to Philippe Mouawad from comment #17)
>
> - How is user supposed to use the Proxy. He starts it in non gui mode. But
> how does he saves the Test plan recorded ?

That's far beyond the scope of this bug.  Think of it as an exercise for the reader. ;)

> I suppose you have some custom development to do that ? Any chance you
> contribute more ?

I'd *like* to, but we've been over this before: I can't actually release/upstream our full headless recording daemon until the release has been cleared with legal.  I'm still making noise about it, but it's not up to me and I don't have an ETA.  I'm in a bit of a grey area already for cleaning up that patch and sample.

> - Would it make sense in the future to have a command line option to start
> proxy in NON GUI mode

Maybe.  In practice, we've found it works best to have the recorder be a daemon that accepts commands from a TCP client and gives feedback. (Similar to how jcmd or jfr work, I think.)

BTW, can you be update the milestone?  I don't have permissions for that.
Comment 19 Felix Schumacher 2016-10-20 20:16:05 UTC
Created attachment 34394 [details]
Allow to use ProxyControl in non GUI mode

Use instance variable instead of static one.

@Wyatt
Was there any reason to use a static variable? Can you check, if this patch helps you?

@Team
I think it is safe to include this in 3.1, what do you think?
Comment 20 Wyatt Epp 2016-10-20 21:16:41 UTC
(In reply to Felix Schumacher from comment #19)
>
> @Wyatt
> Was there any reason to use a static variable? Can you check, if this patch
> helps you?

That's just how the original patch posted by the reporter did things-- all I did was fix up his patch to apply clean on the 3.0 source tree.

If I had to guess it's just because he thought it was more natural to set things up by calling a static method.  (I happen to disagree.)  I'm pretty sure it'll be fine if it's not static.

> @Team
> I think it is safe to include this in 3.1, what do you think?

We would appreciate it greatly.
Comment 21 Philippe Mouawad 2016-10-20 21:22:10 UTC
(In reply to Felix Schumacher from comment #19)
> Created attachment 34394 [details]
> Allow to use ProxyControl in non GUI mode
> 
> Use instance variable instead of static one.
> 
> @Wyatt
> Was there any reason to use a static variable? Can you check, if this patch
> helps you?
> 
> @Team
> I think it is safe to include this in 3.1, what do you think?

Hi Felix,
Ok for inclusion provided we provide the code sample to use it somewhere in docs.
Note this means we make classes mentionned kind of public API that should then be backward compatible.
Finally I don't see currently how users using code can easily save the recorded test plan.

Thanks for review and patch
Regards
Comment 22 Wyatt Epp 2016-10-20 22:11:43 UTC
(In reply to Philippe Mouawad from comment #21)
> Note this means we make classes mentionned kind of public API that should
> then be backward compatible.

Eh?  The whole API is already public (if largely undocumented)-- I wouldn't have been able to write any of our tooling otherwise.

> Finally I don't see currently how users using code can easily save the
> recorded test plan.

That's because there isn't one.  For all that it only ends up being about twenty lines of code, it was really annoying to write that part, in fact.

(It would be quite grand if there were a simple way to dump a JMeterTreeNode or JMeterTreeModel to JMX via SaveService.  But there isn't.  Has anyone filed a bug about that...?)
Comment 23 Felix Schumacher 2016-10-28 17:59:42 UTC
@philippe I put a sample class inside the test src dirextory. It is not complete in the sense, that it outputs correct code, but it will hopefully give the right direction for those who are interested in this "feature".

Date: Fri Oct 28 17:56:25 2016
New Revision: 1767048

URL: http://svn.apache.org/viewvc?rev=1767048&view=rev
Log:
Remove dependency of ProxyControl on GuiPackage.
Based on patches by jarek102 at gmail.com and Wyatt Epp (wyatt.epp at gmail.com)

Bugzilla Id: 57305

Added:
    jmeter/trunk/test/src/org/apache/jmeter/protocol/http/proxy/NonGuiProxySample.java   (with props)
Modified:
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/proxy/ProxyControl.java
    jmeter/trunk/xdocs/changes.xml
Comment 24 Lars H 2016-11-02 15:23:55 UTC
This is really cool stuff! 

It would be very useful for me (and others too, I think) if this could be exposed as a script, useable out of the box (similar to bin/mirror-server).

Perhaps it could trigger writing the test plan on receiving SIGINT/CTRL-C from the user.
Comment 25 Wyatt Epp 2016-11-02 15:31:20 UTC
(In reply to Lars H from comment #24)
> 
> It would be very useful for me (and others too, I think) if this could be
> exposed as a script, useable out of the box (similar to bin/mirror-server).

I'd recommend filing a bug about that separately.
Comment 26 Lars H 2016-11-03 09:58:12 UTC
(In reply to Wyatt Epp from comment #25)
> (In reply to Lars H from comment #24)
> > 
> > It would be very useful for me (and others too, I think) if this could be
> > exposed as a script, useable out of the box (similar to bin/mirror-server).
> 
> I'd recommend filing a bug about that separately.

Yes that makes sense. Added separate issue for the script: https://bz.apache.org/bugzilla/show_bug.cgi?id=60335