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

Added gem downloads count #212

Merged
merged 5 commits into from
Jul 14, 2014
Merged

Added gem downloads count #212

merged 5 commits into from
Jul 14, 2014

Conversation

bogdanRada
Copy link
Contributor

This was requested in previous pull request : #211

Please let me know if this is ok!. I tested and seems to be working ok . Thank you

@bogdanRada
Copy link
Contributor Author

also i think that this application needs to send headers for Pragma: no-cache (used by HTTP 1.0 and IE browsers)
and Expires: -1 (this is for proxies) . The expires header is needed because Github is caching images now , and i tested this while making my application https://github.com/bogdanRada/ruby-gem-downloads-badge . And only if we set also the Expires header with value -1 the images won't be cached.

I can add them myself, but i wanted to ask you if that would be ok before i do it. Please let me know . Thanks

@espadrine
Copy link
Member

Thanks a lot!

Other download-related URLs on Shields.io are of the form /vendor/dt/… for total downloads and /vendor/dm/ for monthly downloads. I see how to do total downloads, data.downloads. I'm not sure what data.version_downloads stands for. I'm not sure how to get to monthly downloads either. Any ideas?

We're sending no-cache on line 142, along with no-store and must-revalidate. When I tested it, those were enough to prevent GitHub from caching them too strongly. They still used camo, but the images were updated every time. I'd have to check whether that is no longer the case, unless you want to do it of course! If that's the case, we should open a new issue.

@bogdanRada
Copy link
Contributor Author

About the rubygems api

rubygems api doesn't provide information about downloads per month yet or for any other time frame, maybe will be in the future available this information.

Right now the api provides three urls for getting download count

  1. /api/v1/gems/#{@gem_name}.json -> this is the url i used , and it only provides two data attributes
    • version_downloads -- the download count for the latest version ( stable or unstable)
    • downloads -- the total downloads of the gem.
  2. /api/v1/downloads/#{@gem_name}-#{@gem_version}.json -> this is url to fetch the download count for a specific version ( e.g. 4.1.1) and provides almost the same attributes as the one above
    • version_downloads -- the download count for the version provided
    • total_downloads -- the total download count for the gem
  3. /api/v1/versions/#{@gem_name}.json -> this returns a list with all the versions of the gem. Usually this methos is useful for detecting the last stable version. but each item from the list has only one attribute
    • "downloads_count" with the downloads count for each of the version. The other attribute is not provided since it is easy to calculate.

If you consider i should use this other two api methods i can do it , just let me know. I am using all three of them in my application. I just didn't considered them necessary here.

About the cache problem

I saw that you're sending the Cache-Control header, but when i developed my application, using just the Cache-Control header didn't helped, not even if i added a Etag header and a Last-Modified header,
The only combination that worked fine was adding the Pragma header and the Expires Header.

And if you read the documentation from Fastly, you will see that there is a prioritization in how the headers are used. They also use this header

Surrogate-Control: max-age=(time in seconds)

but this can be replaced by

Cache-Control: s-maxage=(time in seconds)

The difference is that the second one is not stripped and is respected by Fastly caches. Problem is not all browsers know about "Cache-Control : s-maxage = (time in seconds) " directive. So it is said in the documentation that it is better to use also

Cache-Control: max-age=(time in seconds)

which works better with most of the browsers.

So what did worked for me was this:

 Cache-Control: no-cache, no-store, must-revalidate, max-age:0
 Expires: -1
 Pragma: no-cache.

I am not sure if this was a problem with Sinatra only, but for me only this worked. That's why i considered that i should let you know about it. If you consider there are not necessary, it's fine with me.

You can also read this github/markup#224, and see that a lot of people reported this problem.
And here is a link to Fastly documentation: https://fastly.zendesk.com/entries/23358013-Intro-to-Caching-and-CDNs

@bogdanRada
Copy link
Contributor Author

Update: Searching again through the api seems there is possible to get the downloads count for a time frame by day using this api url

http://rubygems.org/api/v1/versions/#{@gem_name}-#{@gem_version}/downloads/search.json?from=03/10/2014&to=07/10/2014

or this:

http://rubygems.org/api/v1/versions/rails-4.1.0/downloads.json

The second one returns also the downloads count by day, starting from today - 89.days until today

The result looks like this :

{
2014-10-03: 0,
2014-10-04: 0,
2014-10-05: 0,
2014-10-06: 0,
2014-10-07: 0
}

So i could get the downloads count for a time frame if i add this numbers . But i am not sure this actually works, from what i can see it returns zero for all dates, which is impossible , especially if we think about the gem "rails" which i think was downloaded at least one time in the past 90 days

And this api methods are not listed on their public page http://guides.rubygems.org/rubygems-org-api/ , I only discovered them by loooking at source code, so probably there are not supported

@espadrine
Copy link
Member

Wow, that is outstandingly exhaustive information!

Ideally, all badges should have the same meaning of "total downloads". Unfortunately, this might not be achievable, given the APIs at hand. For the record, Packagist's total downloads does not seem version-dependent, nor does NuGet's (Chocolatey's).

If we should have two "total download" badges with differing meanings, that difference should be made obvious in the badge. Currently, for instance, we have [downloads | 3M total]. We could also have something like [downloads | 3M version total]. What do you think of that?

I applaud your digging through code to find the date-range API. Is there a rubygems.org contributor we can ping about future hopes of fixes for this API?

@bogdanRada
Copy link
Contributor Author

Honestly, I agree with the total download badge to look like this
[downloads | 3M total]
But the second one since it represents the downloads count just for the latest version, I would suggest to keep it like this

[downloads | 3M ]

Because i think having in the same sentence the words "version" and "total" can be confusing . I think this way is better, but is just my opinion. I am ok however with any decision that you make , just let me know.

Also i have one more question. You previously suggested that the url for getting the total downloads should be /vendor/dt/ . How should the url look like for the version downloads count ? Just please let me know and i'll update this PR with the changes.

And about your last question...if there is a contributor of rubygems.org that we can ping about this ...i honestly don't know, however this problem with the date range api is listed in their issue list rubygems/rubygems.org#616 , and from what i can see they decided to remove the functionality , that's why probably there are not listed on their public documentation. I can submit a new issue there, but since they decided to remove it , i am not even sure if this can be considered an issue.

@espadrine
Copy link
Member

Because i think having in the same sentence the words "version" and "total" can be confusing .

You're probably right! Having nothing, however, makes the exact meaning of the badge unclear. Maybe "(latest version)"? Then again, anyone can change the first label for the badge using ?label=latest version like this: http://img.shields.io/packagist/dt/doctrine/orm.svg?label=latest%20version. I don't really have a strong opinion on this, go with what you think most people will like best.

As for the URL, I think the least risky for future compatibility is /vendor/dtv (for "download total — (latest) version"), as it allows potentially subtle corresponding badges for monthly downloads (/vendor/dmv), etc. Besides, it also opens the door for version-specific downloading information, through /vendor/dv/:version/m/….

By the way, I made a new issue rubygems/gems#22 to ask for a way to get monthly downloads from the API.

@bogdanRada
Copy link
Contributor Author

Yes, i agree . Having nothing can be confusing also but my point was not about the label , but about the right side of the badge, the text after the number of downloads. I only suggested to have nothing after that number because looking at other sites like http://www.gemetric.me/ they are doing it in the same way. But was only a suggestion.

But i like your suggestion also. Something like this could look nice:

[downloads | 3M latest version ]

and if someone will have a better idea, they can submit a issue afterwards and that text can be adjusted accordingly. So i guess i am going to go with your suggestion. Seems rather more clear.

Thanks for having the time to answer my questions. I really appreciate your help. I will do the necesary changes tomorrow and submit this new changes to this PR.
Also thanks for having the time to submit that issue to rubygems. I would be really nice if those api methods will be re-enabled. Thanks

@bogdanRada
Copy link
Contributor Author

I made the requested changes, please let me know if it is ok. I added my mistake the headers for Pragma and Expires, but i deleted them afterwards. Sorry for that. Thanks for your help

@espadrine
Copy link
Member

This looks excellent!

I noticed there was a downloads_type parameter in the total downloads URL. Is this for a future patch? Should I keep it?

I'm really eager to merge this!

@bogdanRada
Copy link
Contributor Author

forgot to edit that line. Fixed it now .

@espadrine espadrine merged commit baf95cd into badges:master Jul 14, 2014
@espadrine
Copy link
Member

Thanks a lot! It is live now!

@bogdanRada
Copy link
Contributor Author

thanks : )

@espadrine
Copy link
Member

I'm researching the no-cache stuff. Apparently, sending Pragma: no-cache is only specified as an HTTP request, not as an HTTP response, so it isn't actually useful here.

@bogdanRada
Copy link
Contributor Author

you could test with older versions of IE that uses HTTP 1.0. Using HTTP 1.1 , it's not necessary. Thing is older versions of IE have by default enabled only HTTP 1.0 . This header is only to prevent the browser for caching the badge. But i agree newer versions don't need this anymore. but it's really your decision. And is not so important so i think we can just overlook this if that is your decision.

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.

2 participants