120537 - Dependency graphs take forever to load, and are larger than necessary

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[John Vandenberg]

The dependancy graphs on my bugzilla/webdot (1.7.14) installation take forever
to load, and once loaded the image is twice the size it needs to be.  Bugzilla
and Netscape 4.x both end up showing a black image after several failed attempts
to scroll around the image, and IE does not even finish loading the image.

Comment 1

[Gervase Markham [:gerv]]

Any chance of me blaming this on the webdot server? ;-) Or is there some way we
are calling it incorrectly?

The fact that complicated charts don't render is known; there's not much we can
do about that.

Gerv

Comment 2

[John Vandenberg]

I replaced the webdot call with two calls to 'dot', and all works well now. 
Complicated graphs are not a problem now either, tho I have not stress tested
this.  Is this an acceptable solution for b.m.o, or should I wrap it in a param
before submitting a patch?

Comment 3

[Gervase Markham [:gerv]]

What is "dot"?

Gerv

Comment 4

[John Vandenberg]

dot is the graphviz binary which I presume is doing the work behind webdot.
See http://www.graphviz.org/cgi-bin/man?dot for more details.  The biggest
difference with calling dot would be the usage of b.m.o (or similar open source
sites) disk space, and the increased processing power of generating the images.

Comment 5

[Gervase Markham [:gerv]]

So you are saying that we should create an additional dependency for Bugzilla -
on "dot" - in order to allow the graphs to scale better?

Gerv

Comment 6

[John Vandenberg]

Its not really an additional dependancy, as it would remove the requirement for
'webdot', or it could be coded as a parameter so the maintainer can decide
whether to use 'dot' or 'webdot'.  On installations where Bugzilla and webdot
would be installed on the same machine, it makes more sense to have Bugzilla
generate the graphs itself, rather than using the external CGI webdot, which
then pulls down the raw dependancy file via http.  webdot does not work over
https either, meaning that a https installation needs to allow http connections
thru to data/webdot/*.dot

Another option is to use the perl module Graphviz, which can output as_canon to
create the raw files which Bugzilla currently builds manually, or
as_(png|svg|etc) to create the images directly.

Comment 7

[Andreas Franke (gone)]

I think another perl module wouldn't be a problem, would it? Maybe it's possible
to fail gracefully if it's not installed?

Comment 8

[Aaron Brick]

funny, on mozilla they time out for me. guess i'll go back to using netscape. ;)

Comment 9

[Gervase Markham [:gerv]]

CCing justdave for his views on the "dot" dependency.

Gerv

Comment 10

[Dave Miller [:justdave]]

webdot (the server) is unreliable (research.att.com's version).  It also 
doesn't work over HTTP/1.1, which kills virtual hosting. (which is why they 
don't work on b.m.o, because bugzilla is a virtual host on mothra)

Since it's already an optional feature anyway, I see no reason not to offer a 
local solution to it.  In fact, I'd feel MORE comfortable offering a local 
solution to it than in requiring the use of AT&T's server, even if they do 
allow free public use of it.

Comment 11

[John Vandenberg]

Is it sufficient to use 'dot', for which I have a patch already, and spawn
another bug to re-write the script using the GraphViz module?  I still need to
do a few things like cleanup .map and .png files and wrap it in a Param. 
Anything else?

Comment 12

[John Vandenberg]

Attachment: patch to use 'dot', turned on with webdotbase = 'usedot'

I re-used the existing param as the list is getting long, and both methods are
provided by the same external project.

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 66432 [details] [diff] [review]
patch to use 'dot', turned on with webdotbase = 'usedot'

Ooh, I love the simplicity of this. :-)

I do have a few nits to pick, however...

>Index: defparams.pl

>"This is the URL prefix that is common to all requests for webdot.
[...]
>If you have an installation of bugsplat that hides behind a firewall

"bugsplat"?  Isn't that what Bugzilla replaced at mozilla.org?	Wonder
how we missed this one.  As long as we're tweaking the description,
let's fix that, too. :-)

>Optionally a value of \"usedot\" will attempt to use the \'dot\'
>binary available as part of
><a href=\"http://www.graphviz.org\">GraphViz</a>",

Not sure I follow this.  I know what it means because I see the patch,
but maybe it needs to be explained better.  Maybe we should just nuke
all the part about "install a copy of webdot behind your firewall" and
explain this part instead of that, since that's essentially what you're
doing, and removing the HTTP relay out of the middle of the equation.


>Index: showdependencygraph.cgi

>+        system("dot -Tpng -o $pngfilename $filename");
>+        system("dot -Timap -o $mapfilename $filename");

Ewww...  single-parameter system calls.  Although we are generating the
values for all these variables ourselves, so this is technically safe,
our tests require multiple parameters just as a safety precaution.

Also, we can't depend on "dot" being in the PATH.  We have no clue what
PATH will be set to when the webserver runs, and in fact, globals.pl
even nukes PATH when it starts because it avoids a lot of taint warnings.

What I would suggest here is instead of putting "usedot" as the flag for
local use, put the full path to the dot executable in the param.  Test
for local vs. remote by looking for the "http:" at the beginning of it.

Comment 14

[John Vandenberg]

Attachment: v2

I have totally re-written the the param text to be more concise, as there are
now two simple solutions, the default value, and a file path.  Someone
requiring a more complicated solution should know what their problems are
better than us.

Comment 15

[Stephen Lee]

Given that the imagemap is available at page generation time when running dot 
locally, perhaps it could be tweaked to be included as an inline imagemap 
within the output html allowing for client-side imagemap processing?

Comment 16

[John Vandenberg]

Personally, I would consider that another issue, as doing so alters the manner
in which dependancy graphs are delivered, rather than improving the effeciency.  
I would be happy to implement that improvement tho.

Comment 17

[Gervase Markham [:gerv]]

This patch looks OK to me, but I don't have the ability to test it. Can you
point to some directions for installing "dot" locally?

Gerv

Comment 18

[John Vandenberg]

Gerv, http://www.graphviz.org/pub/graphviz/ contains source tarballs, redhat
rpms and windows binaries.  SuSE has graphviz rpms on its CDs.

Comment 19

[Gervase Markham [:gerv]]

Attachment: Patch v.3

I've tested this. It's fine (in fact, it makes dependency graphs work for me),
except that the param text looks ugly. Consider using a <ul> and changing the
text as follows:

"It is possible to show graphs of dependent bugs. You may set this parameter to
any of:
<ul>
<li>A complete file path to \'dot\' (part of <a
href=\"http://www.graphviz.org\">GraphViz</a>). This will generate the graphs
locally.</li>
<li>A URL prefix pointing to an installation of the <a
href=\"http://www.research.att.com/~north/cgi-bin/webdot.cgi\">webdot
package</a>. This which will generate the graphs remotely.</li>
<li>A blank value. This will disable dependency graphing.</li>
</ul>
The default value is a publically-accessible webdot server."

I attach a version which patches cleanly.

You'll also need to file a bug on documentation to get the download location of
GraphViz put in the Bugzilla Guide.

Gerv

Comment 20

[John Vandenberg]

Attachment: Patch v.4

Modified the param text as per Gervs comments.	As the param text no longer
contains scenarios where webdot fails, perhaps this should also be documented?

Comment 21

[Gervase Markham [:gerv]]

Comment on attachment 67674 [details] [diff] [review]
Patch v.4

r=gerv when you file that documentation bug.

Gerv

Comment 22

[John Vandenberg]

Bug 123394 filed.

Comment 23

[Gervase Markham [:gerv]]

Moving to 2.16 blocker status. This only needs second-review, and when it gets
it, I can do the templatisation. I'd rather not do the templatisation first and
break the patch - that would be rude.

Gerv

Comment 24

[Dave Miller [:justdave]]

ooh, mach-o executable for Mac OS X, too :-)

Comment 25

[Dave Miller [:justdave]]

Comment on attachment 67674 [details] [diff] [review]
Patch v.4

it works!  and it works great!

except for one thing...

If you have .htaccess enabled in checksetup.pl, the permissions granted by the
.htaccess file preclude access to the webdot directory by anyone other than
research.att.com :-)

The default .htaccess files for both the data directory itself and the
data/webdot folder had to be modified for me to be able to see any graphs.

Comment 26

[Dave Miller [:justdave]]

Attachment: Existing patch plus modifications to .htaccess for data/webdot

There's no changes in this patch to the defparams or showdependencygraph.cgi
portions of the patch, I only added a patch to checksetup.pl which corrects the
.htaccess file to allow access to the graphics and map files.

This will need to be relnoted, since it won't overwrite an existing .htaccess
file, so you'll have to delete your existing data/webdot/.htaccess (or move it
out of the way) and let checksetup.pl generate a new one.

Comment 27

[Bradley Baetz (:bbaetz)]

Comment on attachment 70647 [details] [diff] [review]
Existing patch plus modifications to .htaccess for data/webdot

>-    foreach my $f (glob("data/webdot/*.dot")) {
>-        # Here we are deleting all old files. All entries are from the
>-        # data/webdot/ directory. Since we're deleting the file (not following
>-        # symlinks), this can't escape to delete anything it shouldn't
>-        trick_taint($f);
>+    foreach my $f (glob("data/webdot/*.dot data/webdot/*.png data/webdot/*.map")) {
>         if (ModTime($f) < $since) {
>             unlink $f;
>         }


You can't remove the trick_taint - this will cause the unlink to fail. You
won't see this unless you
have old img files, which is probably why it passed tests.

Comment 28

[John Vandenberg]

Attachment: Patch v.6

> This will need to be relnoted, since it won't overwrite an existing .htaccess

> file, so you'll have to delete your existing data/webdot/.htaccess (or move
it
> out of the way) and let checksetup.pl generate a new one.

checksetup now informs the user how to resolve the problem, as well as checking
the graphviz binary path is correct.

Comment 29

[Gervase Markham [:gerv]]

Comment on attachment 71136 [details] [diff] [review]
Patch v.6

>-        print "ok: found v$sql_vers\n\n";
>+        print "ok: found v$sql_vers\n";

If you can explain this, then r=gerv. :-)

Gerv

Comment 30

[John Vandenberg]

That was to group the MySQL/GraphViz lines together.

Comment 31

[Gervase Markham [:gerv]]

Then do you not need to add another \n somewhere below the GraphViz line?

Gerv

Comment 32

[John Vandenberg]

It is at the end of the following block.

Comment 33

[Gervase Markham [:gerv]]

Reassigning to patch author. The current patch has r=gerv, and has been tested
by justdave. Dave - can you give this a second r=?

Gerv

Comment 34

[Dave Miller [:justdave]]

Comment on attachment 71136 [details] [diff] [review]
Patch v.6

it's going to have problems on Windows, but it's the same old issues that were
already there, and will need to be fixed more globally later.

Comment 35

[Gervase Markham [:gerv]]

Comment on attachment 71136 [details] [diff] [review]
Patch v.6

After testing this, I'm afraid I'm going to have to rescind a review :-( 
Please respond to these comments as quickly as you can, as I really want
to start working on the dependent bugs.

>-# Allow access to nothing in this directory except for .dot files
>-# and don't allow access to those to anyone except research.att.com
>+# Allow access to .dot files only to research.att.com 

Why is this wording an improvement? It seems much less clear to me.

>+# Allow access to .png, .gif, .jpg, and .map files in case of a
>+# local dot utilty. 

This needs to say:

>+# Allow access by a local copy of 'dot' to .png, .gif, .jpg, and 
>+# .map files 

>@@ -902,7 +920,7 @@
>     # Check what version of MySQL is installed and let the user know
>     # if the version is too old to be used with Bugzilla.
>     if ( vers_cmp($sql_vers,$sql_want) > -1 ) {
>-        print "ok: found v$sql_vers\n\n";
>+        print "ok: found v$sql_vers\n";
>     } else {
>         die "Your MySQL server v$sql_vers is too old./n" . 
>             "   Bugzilla requires version $sql_want or later of MySQL.\n" . 
>@@ -939,8 +957,38 @@
> 
> 
> 

Nit: one too many blank lines here.

>+###########################################################################
>+# Check GraphViz setup
>+###########################################################################
> 
>+#
>+# Check if we are using webdot or a local 'dot' binary
>+#

This comment does not relate at all to the code it is next to.

>+
>+if(-e "data/params") {
>+  require "data/params";
>+  if($::param{'webdotbase'} =~ /\//) {
>+    # version checking requires generating and parsing dot output
>+    # which is not warrented until defective version are identified

This comment also makes no sense. This code does not generate and parse
dot output. Also, what is a "defective version"?

>+    printf("Checking for %15s %-9s ", "GraphViz", "(any)");
>+    if(-e $::param{'webdotbase'}) {
>+      print "ok: found\n";
>+    } else {
>+      print "not found: $::param{'webdotbase'}\n";
>+    }

Why do you print the webdotbase URL here? In general, if you run checksetup.pl
it looks very ugly, and this needs to be fixed.

>+    # Check .htaccess allows access to generated images
>+    open HTACCESS, "data/webdot/.htaccess";
>+    if(! grep(/png/,<HTACCESS>)) {
>+      print "Dependancy graph images are accessible.\n";
>+      print "Delete data/webdot/.htaccess and re-run checksetup.pl to rectify.\n\n";

Again, I do not understand this.
a) We don't normally print messages when something is found to be correct, and 
b) if it's correct, why do you offer a method to "fix" it?

Gerv

Comment 36

[John Vandenberg]

> Why is this wording an improvement? It seems much less clear to me.

The access requirements have changed and thus the comment should also.  To make
it more clear perhaps it should be "Allow access to .dot files only from
research.att.com"

>+  if($::param{'webdotbase'} =~ /\//) {

This line should be !~ /^http:/ , to check whether the administrator has
specified a local path or not.

> This comment does not relate at all to the code it is next to.

+# Check if we are using webdot or a local 'dot' binary
and ", verify the local 'dot' binary exists and that images are accessible."

> This comment also makes no sense. This code does not generate and parse
> dot output. Also, what is a "defective version"?

The intention was to check and report which version of 'dot' is installed,
however to report the version of dot, one must generate and parse its output,
which is not warranted until Bugzilla depends on a particular version.

Checking, reporting and associated comment can go.

> Again, I do not understand this.
> a) We don't normally print messages when something is found to be correct, and 

>+      print "Dependancy graph images are accessible.\n";

should be 

>+      print "Dependancy graph images are not accessible.\n";

Comment 37

[John Vandenberg]

Attachment: v7: review fixes

Comment 38

[David D. Kilzer (ddk)]

Comment on attachment 74192 [details] [diff] [review]
v7: review fixes

+      print "Dependancy graph images are not accessible.\n";

"Dependency" does not have an "a" in it.  :^)

Comment 39

[John Vandenberg]

Attachment: v8: spelling fix

Comment 40

[Bradley Baetz (:bbaetz)]

I'll play with this in a bit, but I'm worried that the system call will cause
the same windows problems that we currently have with processmail, where cmd.exe
isn't in the path.

As an aside, I'd love us to be able to use the perl GraphViz module - is there a
reason we don't? Although that just calls 'dot', which may have the same
problems with the PATH, with no way to override it. Hmm.

Comment 41

[Dave Miller [:justdave]]

not sure if CMD.EXE would come into play here at all...  since the user is
providing the full pathname to the file, and it's a standalone executable and
not a shell script.  Then again it might, depends on how system() on Win32
processes that kind of stuff.  We can make a note of this possible situation and
resolve that for 2.18.  I'm not too concerned with Win32 compatibility for 2.16
(it would be nice, but everyone's used to hacking Bugzilla for Win32 already
anyway, and we do have Win32 compatibility as one of the goals for 2.18).

Comment 42

[Gervase Markham [:gerv]]

Comment on attachment 74210 [details] [diff] [review]
v8: spelling fix

Much better :-) r=gerv.

Gerv

Comment 43

[Bradley Baetz (:bbaetz)]

Comment on attachment 74210 [details] [diff] [review]
v8: spelling fix

>Index: checksetup.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>+if(-e "data/params") {
>+  require "data/params";
>+  if( $::param{'webdotbase'} && $::param{'webdotbase'} !~ /^http:/ ) {
>+    printf("Checking for %15s %-9s ", "GraphViz", "(any)");

Why do you need a printf here - the values are constant.

>+    if(-e $::param{'webdotbase'}) {
>+      print "ok: found\n";
>+    } else {
>+      print "not found: $::param{'webdotbase'}\n";
>+    }
> 
>+    # Check .htaccess allows access to generated images
>+    open HTACCESS, "data/webdot/.htaccess";
>+    if(! grep(/png/,<HTACCESS>)) {
>+      print "Dependency graph images are not accessible.\n";
>+      print "Delete data/webdot/.htaccess and re-run checksetup.pl to rectify.\n";
>+    }
>+    close HTACCESS;
>+  }

Why only run the htaccess check if data/params exists? Although I guess its ok.

>+    my $webdotbase = Param("webdotbase");
>+    if($webdotbase =~ "/^http:/") {

This shouldn't be in quotes - the stuff below here never gets runs, and the
param is
always treated as local.

Comment 44

[John Vandenberg]

> Why do you need a printf here - the values are constant.

I didnt see performance as an issue with a setup script, thus left the line with
the same structure as the other two similar lines.

> Why only run the htaccess check if data/params exists? Although I guess its ok.

The administrator can not have enabled a local 'dot' config without this file,
and with the relnote I didnt think this check should be intrusive to
installations which are running fine.

Comment 45

[Bradley Baetz (:bbaetz)]

The other lines are all prints. I'm not too fussy, though

Anyway, if you fix that regexp issue so that the server url works, r=bbaetz

Comment 46

[John Vandenberg]

Attachment: v9: review fix

Comment 47

[Bradley Baetz (:bbaetz)]

Comment on attachment 75720 [details] [diff] [review]
v9: review fix

r=bbaetz, but you changed the htaccess error message to say "Dependancy"
instead of "Dependency" in this latest patch. Fix that before checking in.

Comment 48

[Gervase Markham [:gerv]]

Comment on attachment 75720 [details] [diff] [review]
v9: review fix

r=gerv. Zeroj - do you have the ability to check this in yourself?

Gerv

Comment 49

[John Vandenberg]

No sorry I dont.

Comment 50

[Bradley Baetz (:bbaetz)]

Comment on attachment 75720 [details] [diff] [review]
v9: review fix

Actually, after applying this to a clean tree to check it in, the param setup
code needs to check that the path given exists, and warn about rerunning
checksetup if data/webdot/.htaccess exists but doens't allow pngs (Don't error,
since they could have a customised .htaccess)

Comment 51

[John Vandenberg]

Attachment: v10: adds param validation sub-routine

When changing the webdotbase param with an old .htaccess, it will now print:
"Dependency graph images are not accessible. Delete data/webdot/.htaccess and
re-run checksetup.pl to rectify. Changed webdotbase."

I refrained from adding HTML tags to beutify the output, as it would only
hinder bug 133169, which would remove the duplicated validation code.

Comment 52

[Bradley Baetz (:bbaetz)]

Comment on attachment 75892 [details] [diff] [review]
v10: adds param validation sub-routine

>+sub check_webdotbase {
>+    my ($value) = (@_);
>+    $value = trim($value);
>+    if ($value eq "") {
>+        return "";
>+    }
>+    if($value !~ /http:/) {

needs to be /^http:/

>+       if(! -e $value) {

Should be -x, because thats what we really care about. Error messages need to
be changed, and 
checksetup.pl needs this change too.

Sorry for not noticing that one the last time arround.

Comment 53

[John Vandenberg]

Attachment: v11: review fixes

I have changed the three regexp's to allow https, so either absolute URI will
continue to work.  Sorry about the late change.

Comment 54

[Gervase Markham [:gerv]]

Comment on attachment 75898 [details] [diff] [review]
v11: review fixes

r=gerv.

Gerv

Comment 55

[Bradley Baetz (:bbaetz)]

Comment on attachment 75898 [details] [diff] [review]
v11: review fixes

r=bbaetz.

I'll check this in later today.

Comment 56

[Bradley Baetz (:bbaetz)]

Checked in.

Please do file another bug to use the GraphViz module, as well as use a client
side map file, too, while you're at it, so that we don't need these temporary files.

Comment 57

[David D. Kilzer (ddk)]

check_webdotbase() method in defparams.pl should not complain
about images not being accessible if no data/webdot/.htaccess
file even exists.

Additional small patch file to be attached shortly.

Comment 58

[David D. Kilzer (ddk)]

Attachment: Patch Part 2 v.1

Checks for existence of data/webdot/.htaccess before checking
whether it is correct or not in defparms.pl.

Comment 59

[Bradley Baetz (:bbaetz)]

Comment on attachment 77115 [details] [diff] [review]
Patch Part 2 v.1

You should use a new bug....

r=bbaetz anyway

Comment 60

[Dave Miller [:justdave]]

Comment on attachment 77115 [details] [diff] [review]
Patch Part 2 v.1

r= justdave

ddk, you have checkin privs?

Comment 61

[David D. Kilzer (ddk)]

I have no checkin privs.  Should I?

Comment 62

[Gervase Markham [:gerv]]

Checked in.

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.69; previous revision: 1.68
done

Gerv