Skip to content
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

[WIP] Add haxelib downloads per day #432

Open
wants to merge 16 commits into
base: development
Choose a base branch
from

Conversation

flashultra
Copy link

This is WIP . At the moment I add new table Downloads where every haxelib install will be counted per day.
The main problem is how to show this data. It's possible to be use library
https://github.com/fnando/sparkline and data to be added in function infos(...) ( Repo.hx) and show in new tab ( at example before "All Versions " )

@andyli
Copy link
Member

andyli commented Oct 29, 2018

Haxelib is hosted with auto-scaling, so there can be 2 or more instances of haxelib servers running in parallel. I believe getting the download count and then setting it by +1 may result in race condition.

@flashultra
Copy link
Author

In that case are there similar problem with :

      v.downloads++;
      v.update();
      p.downloads++;
      p.update();

i.e. if one instance of haxelib read p.downloads, and before to update , the other one also read the old value of p.downloads the wrong value will be updated ( same as d.num++; d.update() )
Possible solutions will be to lock function postInstall ( not sure how is possible in Haxe) or to move all logic from postInstall to transaction procedure ( how ? ) .

@andyli
Copy link
Member

andyli commented Oct 29, 2018

I've little experience with record-macros so I'm not sure if there is solution for that. I would just use plain SQL (sys.db.Connection#request()) to increment the counter.

@flashultra
Copy link
Author

For me if make unsafeExecute public ( in record-macros ) and after that replace code with
unsafeExecute("INSERT INTO Downloads ( pid, date, num) VALUES (${p.id}, 1, CURDATE() ) ON DUPLICATE KEY UPDATE num = num +1 "); is the right solution and in that case won't have race condition

@flashultra
Copy link
Author

Or maybe the other way to run such query will be Downloads.manager.cnx.request("INSERT INTO ... ");

@andyli
Copy link
Member

andyli commented Oct 30, 2018

Would you add a simple test? e.g. In the integration test, run haxelib install something and then check the counter directly from DB.

@flashultra
Copy link
Author

I added integration test , but it's possible to do something wrong. I could not see example for using dbCnx connection, so my assumption is that connection is still alive. If need to correct something , please tell me.

@andyli
Copy link
Member

andyli commented Oct 30, 2018

According to Travis:

src/haxelib/server/Repo.hx:363: characters 2-23 : Cannot access static field cnx from a class instance

Have you tried running it locally?

@flashultra
Copy link
Author

No at the moment. I should try to run it locally with Docker , looks like at simple solution vs. mod_neko, apache etc.

@markknol
Copy link
Member

At the moment we use https://gionkunz.github.io/chartist-js/ to display chart, where one axis is date of released lib, and other axis is download count. I think we can a different another with the same lib.

But maybe we should first let this run for few months before actually show the data (otherwise it will be confusing).

Another question for the view I have is; How are we going to deal with older downloads from before this PR that don't have timestamps?

@flashultra
Copy link
Author

flashultra commented Oct 30, 2018

Yes, I saw Chartist , but I'm not sure how good will be to represented all information. At example npmjs show max data for a year separate by weeks ( or about 52 points in graph) . But yes , we should try first is it good or not.

About older downloads, if not have any data, graph will be empty, if have , at example, for 3 weeks , only that data will be represented. Unfortunately , it's not possible to use the current download data, because it is based on version release.

@flashultra
Copy link
Author

I tried Chartist with 52 weeks and this is the result. If you point dots will get info for which week is the current download value. I still prefer some sparkline ,but it's not my decision after all.

@markknol
Copy link
Member

markknol commented Oct 31, 2018

Ok thats a good start! We have to make sure its clear if this are stats for one version or all versions, maybe create multiple views for that (one with sum, one with lines for each version)?

@flashultra
Copy link
Author

Well, for me the main idea was to show weekly downloads , so that stats include all versions i.e in that case you can see how the current library is used by users regardless of version for certain period of time.

Not sure how useful will be information for each version, because at first lib can be updated every 1-2 days ( I think OpenFL have frequently updates) and second what information will bring for lib if show week downloads per version ( already have all downloads per version )

Just one example: haxe-crypto have about 250 downloads per week, and hamcrest have about 30 , but hamcrest have more downloads (58902 ) than haxe-crypto ( 58829) , so that mean at the moment more people are interesting from haxe-crypto than hamcrest .

I think this is interesting and useful statistics for that what it's been using in haxe ecosystem and in what need to be invest more.

@flashultra
Copy link
Author

Integration test failed because this rqOne.getIntResult(0) return error exception thrown : mysql5@result_get_int after SELECT num FROM Downloads WHERE pid = ${pid} AND `date` = CURDATE() . There are some possibilities:

  • haxe install Bar not trigger install count
  • num is return as other value
  • getIntResult works by different way

I was trying to run haxelib locally by Docer toolbox ( because I have VirtualBox) , but get this error:
ERROR: Service 'web' failed to build: COPY failed: stat /mnt/sda1/var/lib/docker/tmp/docker-builder247917717/.haxelib: no such file or directory

@EricBishton
Copy link
Member

@markknol I'm not sure I'm following this fully, but it seems like there is missing data (e.g. timestamps for downloads) that isn't available in the web site stats for prior transactions. However, you've been collecting that in a spreadsheet for a while, haven't you? Or was that only compiler downloads?

@andyli
Copy link
Member

andyli commented Nov 1, 2018

For the .haxelib: no such file or directory error, you can solve it by running the commands as follows

haxelib/.travis.yml

Lines 66 to 68 in 0161c1a

- haxelib newrepo || mkdir -p .haxelib && haxelib setup .haxelib
- yes | travis_retry haxelib -notimeout install all
- yes | travis_retry haxelib -notimeout install tora

It is done such that when building the Docker image we don't have to call haxelib install, which for once was broken in production and was painful to deploy a new haxelib server update.

I should have updated the README but forgot about it.

@markknol
Copy link
Member

markknol commented Nov 1, 2018

@markknol I'm not sure I'm following this fully, but it seems like there is missing data (e.g. timestamps for downloads) that isn't available in the web site stats for prior transactions. However, you've been collecting that in a spreadsheet for a while, haven't you? Or was that only compiler downloads?

That is downloads from GitHub releases and indeed only for compiler downloads. This is for haxelibs in its own database.

@flashultra
Copy link
Author

Thanks @andyli . Have some progress , but now the server is not available.

If try to use docker-compose.yml it's redirect me to https and give " Error code: SSL_ERROR_RX_RECORD_TOO_LONG " / "ERR_SSL_PROTOCOL_ERROR"

For dev version docker-compose-dev.yml got 'Forbidden You don't have permission to access / on this server.' and for some reason couldn't change dir attributes.

@flashultra
Copy link
Author

For some reason the following command "haxe integration_tests.hxml" do not call Repo.hx file and for that in Downloads table nothing change.
I simulate connection to database and increase the counter for Downloads directly in TestDownloads.hx .
If someone can tell me what is wrong with integration_test.hxml I would fixed.
In other case you can approve the current state ( and start gather data for downloads ) or reject the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants