New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PiwikTracker.java sync with PHP tracker and release 1.0 #3151
Comments
OK I finally got some time to work on this. My changes are in good shape I think. I should be able to complete testing this week, so I can attach my updated code to this bug report.. In the meanwhile a few comments, with specific questions at the end. Ive fixed the visitor ID logic so it uses cookies like "_pk_id.6.1fff" not "piwik_visitor. This is the only change which is really NEEDED. Ive updated the logic which examines the HttpServletRequest so that it is proxy-server aware. If the user is being a proxy server, which sets X-Forwarded-For, we use that information instead of the IP address from the HttpServletRequest. This way we get the IP address of the user not the IP address of the proxy server. On the version numbering, this version should probably be v1.1 or perhaps 2.0. Versions 1.0.0 through 1.0.5 have been posted under Ticket #2172. Other changes: When checking a visitor ID, Ive checking that we have a 16 character hexadecimal string. This is more strict than the PHP code which just checks the Strings length. The comments indicate that Java 1.5 is required but the code really requires Java 1.6. (It uses java.net.HttpCookie and String.isEmpty) Ive removed the IPiwikTracker interface. I cant think of any scenario where you would want it. Ive cleaned up the API, removing misnamed methods: getDownloadTackURL and getLinkTackURL. Methods which accept a Goal now take an int not a String. The PHP code says revenue is also an int, but in the database it is a float. So I think we should have: public final URL getGoalTrackURL(final int goal, final float revenue) { .. } Implemented methods like doTrackGoal(final int goal, final float revenue) In the sendRequest method, it would silently do nothing if the supplied URL was null. This does not jive with the PHP implementation and doesnt make much sense. Now, if the supplied URL is null, you will get a NullPointerException. I havent made this change yet, but I feel like the PiwikException class is a mistake. It takes all sorts of different problems and lumps them into a single PiwikException class. For example a caller can no longer easily differentiate between IOException (dead network connection) from MalformedURLException (probably bad programming). You can presumably use getCause() but that is awkward. I would much prefer to have a method like setApiurl throw MalformedURLException if the supplied URL is invalid or a raw NullPointerException if it was null. Why obfusticate the error? I would personally return the original exception, for example a MalformedURLException, ParseException or IOException. In some cases there is no original exception but IllegalArgumentException is appropriate in almost all cases. The only case where IllegalArgumentException is not appropriate is when the server does not return a success code. In that case I would add HttpResponseException as an inner class. Questions:
|
thivnor, you can commit directly your changes rather than attaching patches.
|
(In [6411]) Refs #3151 Floats |
Attachment: V1.1 of SimplePiwikTracker.java |
I don't have an account and I'm not up to speed on SVN. So I've just attached my updated file. I've used .zip because my .java file was rejected ... too many URLs made it look like spam. I've put change notes in the file itself. But at a high level I've fixed the bug, supported operation behind a proxy server, made a variety of API changes, and put everything into a single file. So, matt, if you check this in you can remove all the other .java files. The "single file" aspect makes it easy to import, it is just one file so if you want to import it into your code base it is easy to do. To do this, I converted two classes to inner classes, this is easy, and I think appropriate. The "ResponseData" class is "what you get after calling something like doTrackGoal." The "EBrowserPlugins" is also pretty 'internal' to the SimplePiwikTracker class. So I don't feel I'm forcing this ... it feels like a single file is a good fit for what the package is going. This also lines up with the PHP implementation which is a single file. In terms of API changes, the big thing is how exceptions are handled. Rather than a single monolithic "PiwikException" you get specific exceptions. This makes it easy to handle transient problems like IOException differently from programming errors like MalformedURLException. In many cases the class has methods I don't see the value of. The point of the tracker is to track what the real user is doing. So having methods like setResolution, which replace the user's real screen resolution, makes little sense. I've left them in place but don't see the value. I've included JavaDoc comments to be careful with setTokenAuth / setVisitorId, I'm not sure how they would interact. Because of the API changes, the unit tests are totally broken. I have not yet been converted to the 'unit testing religion' as it were. I review the existing unit tests, somewhat quickly, and saw a lot of tests which didn't have much value. If there were a handful of tests which tested the overall class I could see taking the time to get the tests in line with the new class. But so many of the tests are just busy work, I don't see the point. My guess is that piwik.org doesn't use Java-based unit testing in an automated way so this should not be a problem in terms of your build system. If this does create a problem however we could rename SimplePiwikTracker.java to PiwikTracker.java and leave the other code in place. Or, if someone makes a compelling case for unit-testing this class, I could probably the time to get up to speed on JUnit. In terms of my own testing coverage, I am focused on goal-tracking, so that is the specific thing I've tested. I don't expect problems with the other methods however as I haven't messed with their logic. I have not tested the cookie handling: for example if you use a track to track several things, it should get the cookie from the server, save it, and use it in subsequent requests. I've removed logic related to XDEBUG cookies. I don't know what they are but the logic seemed really fishy and likely to break normal cookie handling. If anyone can explain what is up with XDEBUG cookies I can revisit this. I have tested behavior behind a proxy server. The code now correctly reports the IP and request URL, even when behind a proxy server. I've specifically tested against Apache httpd. One odd limitation is that the proxy server doesn't send the original request URL, and I don't know how to determine the original port and protocol used for the request. So if you have a secure proxy server my code will misreport it. My code just assumes the proxy server is using http on port 80. The code is well commented and should be easy to change if it is a problem for anyone. I've used v1.1 not v1.7 as I don't want to imply this is tied to the Piwik version. This code should be fine as long as the REST API stays stable and the cookie format doesn't change. |
Adding everyone from previous tickets. If you use the Piwik JAVA class, please give your opinion on the new code submitted by thivnor in this ticket. Your feedback is appreciated. @thivnor all your changes make sense to me.
Thanks for your changes! Will wait for someone else to confirm and commit |
Thanks thivnor for the great update! I will test it as soon as I have time. |
A couple of important things to note:
And just in general, I think that just because the PiwikTracker.php code all fits in a single page does not mean that the java implementation should fit within a single class file. Call me a java purist or whatever, but there is no separation of concerns (separating visitor info from page info, mixing the html connection stuff into the query building and other logic). And using strings in java to concatenate the query, seriously? I found the code to only be useful as a reference for how it could be done, but certainly not how it should be done. That said, I thank you all for your contributions anyway... |
dnebing, thanks for the code review. A little context may be in order. I don't think this code is widely used. When I started using v1.0.2, I found it very broken. It would record visits, but would not assign them to the correct visitor ID. It was using the wrong cookie to extract the visitor ID, among other problems. The original authors don't seem to be around anymore. So I volunteered to make the code at least basically functional. Or to be more exact: so it would meet my very simple needs. In the process I did some code tidying. For example the exception handling is much more direct now. But this code is not a priority for me. I don't know much about Piwik, and am not particularly proficient in PHP. So, for example, I am unlikely to work on compatibility with the 'ecommerce or goal campaign elements.' Similarly, on 'separation of concerns,' I'm not going to spend a lot of time making the code perfectly tidy. As they say in the Perl community, 'There's more than one way to do it.' Context aside, the typo is easily fixed, as is the issue relating to how Piwik expects booleans to be represented ('0' not 'false'). But what most concerns me here is the issue around cookie names. You note that "The default install of piwik, if third party cookies are enabled, uses only _pk_uid for the cookie." As mentioned, I'm not Piwik guru. I have used PiwikTracker.php as the definitive reference, and it does not appear to respect _pk_uid as a cookie name. The relevant function is getVisitorId, which has:
Can anyone clarify what cookies Piwik will use? |
@thivnor Thanks again for taking over this class and maintaining it for the Piwik Java dev community, it's very appreciated!! Regarding the cookie name, we don't currently use the third party in the PHP tracker so the Java is not expected to do it. Maybe we could add this in the PHP tracker, in which case @dnebing please create a ticket for this separate request. @dnebing please submit a patch & the new zip for ecommerce tracking :) http://piwik.org/participate/development-process/#toc-how-to-submit-a-patch |
I just realized this new code is not in Piwik trunk in: https://github.com/piwik/piwik/blob/master/libs/PiwikTracker/java/src/main/java/org/piwik should we update SVN with your latest code? can you confirm it's working well? Has anyone else tested it ? thanks! |
I checked out the trunk version today and just replaced it with 1.1. I will use it in production soon, and have a high interest that it works, so I'm happy to give it some love. I will do a raw check of the functionality and then report back. On the long haul I would also propose to refactor this class. But until then I'm happy to provide updates and bugfixes. |
In my option the v1.1 I posted is a significantly better than the prior version, simply because it uses the correct tracking cookie. There are clearly numerous potential improvements and fixes. But I do think this is the version to check in. Note that my .zip has a single file, which replaces all the files of the original. So you probably want to delete the other files; they are no longer needed. I have not integrated the two recommendations from dnebing:
CURRENT CODE
RECOMMENDED CHANGE
|
I'm sorry, I am trying to use this java API but I cannot find the doTrackDowlonad method in any of the files attached. Am I missing something? How do you send the tracking request? |
Line 268:
(Sorry "download" is misspelled) |
I extracted the code + history into its own repository on Github: http://github.com/piwik/piwik-java-tracking In case you'd like to make any changes please submit a pull request - they are very welcome! |
Awesome work on the migration @halfdan! Let's close this ticket now, and please report any bug you experience, with the Java tracking API at github https://github.com/piwik/piwik-java-tracking |
and of course if anyone wants write access to the repo on github please make one pull request and ask for it, it would be nice to find official maintainers who are using the API for real projects. |
Follow up from #2172
The goal of this ticket is to improve the JAVA tracker and sync it with the PHP tracker in terms of features and public APIs.
Thank you to all contributors!
Code is available in: "Java Tracking Client"
The text was updated successfully, but these errors were encountered: