Page MenuHomePhabricator

Enable avatars in gerrit
Open, LowPublic

Description

gerrit.googlesource.com shows little face photos next to usernames, which is a good way for making it quicker and less mental effort to recognize / spot people, and also for humanizing the interface. We should do that too.

The simplest solution seems to be to install the avatars-gravatar plugin.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Tgr added a comment.Apr 2 2018, 8:23 AM

Using something like gravatar-privacy-proxy would solve both the privacy and the performance issues.

Using something like gravatar-privacy-proxy would solve both the privacy and the performance issues.

Gravatar still relies on us providing the providing personal details (email address) to find the avatar in the first instance.

But there is also the factor that git at its base design does share email address for those that commit into the repo, But gerrit does also share the ema address in more instances than just committing such as being added as a reviewer to a patchset.

see also:
T151529: Using Gerrit/git requires the email registered via wikitech and ends ups being voluntary disclosed (break of privacy?)
T179212: Create avatar plugin for gerrit that uses phab's conduit to get users profile image

Tgr added a comment.Apr 2 2018, 11:17 AM

The primary email address in gerrit is already public and can be viewed by typing e.g. owner:<username> in the search box.
The registration interface warns about this, so there is no violation of privacy expectations.

Using phab avatars would be nice too, but probably more effort to maintain? APIs are not a strong suit of Phabricator.

demon added a subscriber: demon.Apr 2 2018, 9:17 PM

avatars-gravatar I am not installing. I did once, and was very quickly told not remove it. While that Wordpress proxy plugin solves the problem of performance/caching and exposing your IP to them, it still sends them your e-mail address. Just because e-mails are public doesn't mean we should broadcast them to a third party.

We could very easily write a avatars-commons or some similar plugin. avatars-* plugins are dead simple. A Phabricator one wouldn't be too hard either, barring any major API issues.

I think we could do a avatars-commons or the phab one, but with phab it was slow when we used there conduit.

Change 424708 had a related patch set (by Paladox) published:
[operations/puppet@production] Gerrit: Add url for avatars

https://gerrit.wikimedia.org/r/424708

Im not sure what the default avatar should be, though chad suggested a stick man :)

https://gerrit.wikimedia.org/r/c/425127/

Change 425127 had a related patch set uploaded (by Paladox; owner: Paladox):
[All-Avatars@master] Add default.png (stick figure) as default avatar for gerrit

https://gerrit.wikimedia.org/r/425127

Tgr added a comment.Apr 9 2018, 9:59 PM

I'd go with something slightly more professional, e.g. this.
A plain light grey or light blue background is not a terrible choice either.

demon added a comment.Apr 9 2018, 10:15 PM

Professional? The cloud services logo is a unicorn.

demon added a comment.Apr 9 2018, 10:15 PM

A unicorn, I'll add, that was my suggestion ;-)

Change 425127 merged by Paladox:
[All-Avatars@master] Add default.png image as default avatar for gerrit

https://gerrit.wikimedia.org/r/425127

demon moved this task from Bugs & stuff to Local hacks on the Gerrit board.May 15 2018, 9:08 AM
Paladox removed a subscriber: Traffic.
Restricted Application added a project: Operations. · View Herald TranscriptJun 12 2018, 3:21 PM

Change 439939 had a related patch set uploaded (by Paladox; owner: Paladox):
[operations/puppet@production] Add gerrit.wmfusercontent.org to common/cache/misc.yaml

https://gerrit.wikimedia.org/r/439939

Change 439939 merged by Dzahn:
[operations/puppet@production] cache::misc: Add gerrit backend, gerrit.wmfusercontent.org

https://gerrit.wikimedia.org/r/439939

Dzahn added a subscriber: Dzahn.Jun 13 2018, 10:16 AM

gerrit.wmfusercontent.org now exists in cache::misc and requests would be forwarded to cobalt as the backend.

This unblocked this to a certain extent because avatars could now be hosted on gerrit.wmfusercontent.org (akin to phab.wmfusercontent.org is used as a separate domain for user uploads, for security reasons).

Next would be an Apache virtual host on cobalt to handle it.

gerrit.wikimedia.org itself is _not_ behind cache::misc but serves traffic directly with a public IP.

Dzahn triaged this task as Medium priority.Jun 13 2018, 10:17 AM

Change 439783 had a related patch set uploaded (by Paladox; owner: Paladox):
[operations/puppet@production] Gerrit: Add support for avatars url in apache

https://gerrit.wikimedia.org/r/439783

Change 439808 had a related patch set uploaded (by Paladox; owner: Paladox):
[operations/puppet@production] Gerrit: Hook up gerrit.wmfusercontent.org to apache

https://gerrit.wikimedia.org/r/439808

Change 440104 had a related patch set uploaded (by Paladox; owner: Paladox):
[operations/puppet@production] Gerrit: Clone avatars repo into /var/www/avatars

https://gerrit.wikimedia.org/r/440104

Change 424708 abandoned by Paladox:
Gerrit: Add url for avatars and setups gerrit.wmfusercontent.org

https://gerrit.wikimedia.org/r/424708

@thcipriani or @mmodell wondering if you be able to comment here that releng supports this avatar change and maintaining it please. (Ops need releng to comment)

Change 440104 merged by Dzahn:
[operations/puppet@production] Gerrit: Clone avatars repo into /var/www/gerrit-avatars

https://gerrit.wikimedia.org/r/440104

Change 456437 had a related patch set uploaded (by Paladox; owner: Paladox):
[operations/puppet@production] Gerrit: Setup avatars url in gerrit config

https://gerrit.wikimedia.org/r/456437

Change 439783 merged by Dzahn:
[operations/puppet@production] Gerrit: Add support for avatars url in apache

https://gerrit.wikimedia.org/r/439783

Change 456514 had a related patch set uploaded (by Paladox; owner: Paladox):
[operations/puppet@production] gerrit: Index avatars url showing directory listings

https://gerrit.wikimedia.org/r/456514

Change 456514 merged by Dzahn:
[operations/puppet@production] gerrit: redirect avatars_host root URL to regular gerrit

https://gerrit.wikimedia.org/r/456514

ema moved this task from Triage to Watching on the Traffic board.Sep 5 2018, 8:07 AM

Change 439808 merged by Dzahn:
[operations/puppet@production] Gerrit: Hook up gerrit.wmfusercontent.org to apache

https://gerrit.wikimedia.org/r/439808

Mentioned in SAL (#wikimedia-operations) [2018-10-05T20:44:16Z] <mutante> gerrit - adding gerrit.wmfusercontent.org virtual host for avatars. applied first on gerrit2001, then on cobalt (T191183)

Mentioned in SAL (#wikimedia-operations) [2018-10-05T20:48:52Z] <mutante> T191183 - it's still showing the error page as before but that isn't due to apache issues, it just needs additional ferm rules

Change 464907 had a related patch set uploaded (by Dzahn; owner: Paladox):
[operations/puppet@production] Gerrit: Change backend for gerrit in varnish

https://gerrit.wikimedia.org/r/464907

Dzahn added a comment.Oct 5 2018, 9:44 PM

Yea, it's not ferm, it's the wrong backend IP per change above.

Change 464907 merged by BBlack:
[operations/puppet@production] Gerrit: Change backend for gerrit in varnish

https://gerrit.wikimedia.org/r/464907

Krinkle added a subscriber: Krinkle.EditedOct 6 2018, 3:56 AM

The prior conversation at T179212 may be relevant here. The two options discussed so far were 1) Use Gerrit repo with static file server, or 2) Use Phabricator.

Gerrit repository

Benefits:

  • Easy to start with and operate (static file server, in theory; if everyone behaves).
  • Users can have a different avatar on Gerrit than Phabricator, if they want.
  • Users can have upload an avatar to Gerrit, even if they don't want to have a Phabricator account.

Drawback:

  • By default nobody has an avatar on Gerrit.
  • The workflow is not very discoverable or user-friendly: Clone a git repo, crop/resize an image offline, add to the directory on disk, create and submit a commit, then merge it on Gerrit. Gerrit wants 100x100px square thumbnails. How do we educate about, validate or enforce this - to avoid having 2MB photos being uploaded and downloaded on every one's Gerrit dashboards.
  • Everyone has to maintain their avatar in a second place in addition to Phabricator, which means casual contributors probably won't be able to find out how to do it, or able to, or invest time in doing so.
  • Repo can grow uncontrollably large (per @demon).
Query Phabricator

(via a cacheable CGI script, or from a static directory populated periodically by a cron job script).

Benefits:

  • Everyone with a Phabricator account has an avatar by default. Including those without custom avatars, whom will have their personalised default from Phabricator (based on colors, letters, icons).
  • As a user, no need to maintain avatars in two places.
  • The workflow for creating an avatar is is user-friendly. If you have a Phabricator account, no steps required. If you don't, you can create one and use Phabricator's user interface to upload an image, which is then automatically resized. The act of creating an account and setting a profile picture is presumed to be easier, less time consuming, more accessible and easy to discover via the gerrit.avatars.changeUrl option. – Compared to cloning a repository, resizing an image manually, and committing, pushing and merging a patch).

Drawbacks:

  • Opting out is only possible if you opt-out from a profile picture on Phabricator as well.
  • Cannot have a different profile picture on Gerrit than on Phabricator.
Dzahn added a comment.Oct 6 2018, 12:49 PM

I don't think that cloning / committing / pushing is a big hurdle in this case. After all we are talking about Gerrit users not Phabricator. People who already explicitly come to do code review will be familiar with using git.

note that the avatar would have to match the users username in gerrit. So using phabricator would not work seeing as the name of the file does not match the users username.

Dereckson added a comment.EditedOct 8 2018, 12:13 PM

Gerrit wants 100x100px square thumbnails.

The 100x100 size isn't what currently happens in the repository:

$ identify *.png
default.png PNG 425x425 425x425+0+0 8-bit Grayscale sGray 13916B 0.000u 0:00.007
ladsgroup.png PNG 640x640 640x640+0+0 8-bit sRGB 228c 123552B 0.000u 0:00.000
niedzielski.png PNG 350x350 350x350+0+0 8-bit sRGB 150395B 0.000u 0:00.000
paladox.png PNG 400x400 400x400+0+0 8-bit sRGB 109c 2564B 0.000u 0:00.000

(the size are after optipng -o7 to give a fair representation of what we could expect)

Change 465149 had a related patch set uploaded (by Dereckson; owner: Dereckson):
[All-Avatars@master] Optimize the PNG assets

https://gerrit.wikimedia.org/r/465149

Change 465149 merged by Paladox:
[All-Avatars@master] Optimize the PNG assets

https://gerrit.wikimedia.org/r/465149

[..] using phabricator would not work seeing as the name of the file does not match the users username.

Can you explain what you mean here?

The Phabricator export script does not exist yet. There are no files yet to have names. If and when we write this script, of course, we use the correct file name.

@Krinkle would you know how to build this? Seeing as you can link your mediawiki profile or your ldap account, you could have a different name.

Krinkle added a comment.EditedOct 8 2018, 7:06 PM

@Krinkle would you know how to build this? Seeing as you can link your mediawiki profile or your ldap account, you could have a different name.

Yes, you can. You can also link it to a Developer account (LDAP), which is what Gerrit uses. So the proxy script or export script, would query Phabricator for a user that has the Gerrit name as LDAP link. And if there is one match, use it.

We already talked about this a few months ago on IRC, and at T179212

Ok, but would you know how to write this cgi script?

Yes, I think anyone involved around this task could do it. It's not a question of how.

The question is, what do we want for the user experience, and would it be worth it to write a little script for it or not.

avatars-gravatar I am not installing. I did once, and was very quickly told not remove it. While that Wordpress proxy plugin solves the problem of performance/caching and exposing your IP to them, it still sends them your e-mail address. Just because e-mails are public doesn't mean we should broadcast them to a third party.

Really? As far as I know all gravatar integrations send hashes of emails. This means they don't know which email it is, unless it exists in their system (assuming they don't crawl email addresses from elsewhere). Having said that, it still doesn't sound a very good idea.

hashar added a subscriber: hashar.Oct 9 2018, 1:43 PM

avatars-gravatar I am not installing. I did once, and was very quickly told not remove it. While that Wordpress proxy plugin solves the problem of performance/caching and exposing your IP to them, it still sends them your e-mail address. Just because e-mails are public doesn't mean we should broadcast them to a third party.

Really? As far as I know all gravatar integrations send hashes of emails. This means they don't know which email it is, unless it exists in their system (assuming they don't crawl email addresses from elsewhere). Having said that, it still doesn't sound a very good idea.

There are a few troubles:

  • that still leaks information to a third party
  • the feature now depends upon a service to serve the avatar
  • users sill have to upload their avatar to a third party, create an account there etc

gerrit.avatars.changeUrl and an end point on Phabricator side seems to the easiest solution as well as being user friendly (one place to update it). ver Ideally we would add the information to LDAP to have the avatar at a central place and reused everywhere (wiki, Phabricator, Gerrit), but that is over the current scope.

Tgr added a comment.Oct 9 2018, 8:39 PM

Really? As far as I know all gravatar integrations send hashes of emails. This means they don't know which email it is, unless it exists in their system (assuming they don't crawl email addresses from elsewhere).

Gravatar hashes are plain unsalted md5 so not really useful for protecting your email address - it can just be plugged into one of the reverse email hashing services to get the raw address back.

Gerrit wants 100x100px square thumbnails.

The 100x100 size isn't what currently happens in the repository:

$ identify *.png
default.png PNG 425x425 425x425+0+0 8-bit Grayscale sGray 13916B 0.000u 0:00.007
ladsgroup.png PNG 640x640 640x640+0+0 8-bit sRGB 228c 123552B 0.000u 0:00.000
niedzielski.png PNG 350x350 350x350+0+0 8-bit sRGB 150395B 0.000u 0:00.000
paladox.png PNG 400x400 400x400+0+0 8-bit sRGB 109c 2564B 0.000u 0:00.000

(the size are after optipng -o7 to give a fair representation of what we could expect)

This is probably something we should enforce somehow (jenkins? some tool to be created to upload?) before exposing this feature broadly: optimization of uploaded images and enforcement of the size of these images to help mitigate the problem of this repo growing "uncontrollably large".

Also, should we be using git lfs (or maybe just not git at all) for this repo? The files themselves are pretty tiny, but I don't think there's any advantage to storing old avatars as binary data in a repo folks are expected to clone and push. For instance, the optimization patch above, while necessary for serving these images on the web actually made the repo itself larger since now we have the old default.png (28K .git/objects/ec/e17874be5f0ae77c25c0dd672a3c71eebb536c) and the new default.png (14K .git/objects/94/d4ec77c43cab9d3d9952ca00c46a4e8878f711). Having lots of users use this repo and having lots of users updating their avatars over time seems like it would make this repo hard to work with and generally fulfill the "uncontrollably large" prophecy.

greg added a subscriber: greg.Oct 10 2018, 5:17 PM

This is probably something we should enforce somehow (jenkins? some tool to be created to upload?) before exposing this feature broadly

Yes.

Also, should we be using git lfs (or maybe just not git at all) for this repo?

Yes.

We won't be able to use git lfs on cobalt as it is using jessie whereas the git-lfs package is in stretch-backports+.

We could enforce it so users can only upload there image through a tool hosted on tool forge. So that way it can run the image optimising tool and upload.

We could modify this https://github.com/valhallasw/gerrit-patch-uploader/ or fork it.

greg added a comment.Oct 10 2018, 10:20 PM

We won't be able to use git lfs on cobalt as it is using jessie whereas the git-lfs package is in stretch-backports+.

That's simply a technical issue to work through, not a complete blocker.

We could enforce it so users can only upload there image through a tool hosted on tool forge. So that way it can run the image optimising tool and upload.

We could modify this https://github.com/valhallasw/gerrit-patch-uploader/ or fork it.

This goes back to Krinkle's question of what we want the user experience to be.

Tgr added a comment.Oct 10 2018, 11:15 PM

UX-wise a single central place for profile images is obviously preferable, so using Phabricator makes a lot of sense. (Having some way to store a profile image in your Wikimedia central account would make even more... maybe in the next decade :) OTOH that would mean you can't use a different image, which could be problematic as the avatar size is very different between Phabricator (several sizes, but most commonly 50x50 in comment threads and 140x140 on hover) and Gerrit (16x16) so the same image might not work well.

Also, Conduit is not confidence-inspiring though, IMO - poorly documented (if at all) and in constant churn. Most importantly, it's not very fast, so the Gerrit plugin would still have to store images somewhere. At that point, maybe just cut out Phabricator and manage that store directly, and make it easy to transfer the Phabricator profile image? (I agree that git is not the most convenient or frugal backend for that.)

If gerrit only shows 16x16 images, I would argue that it's not even worth doing. At that size, avatars will be difficult to distinguish and that will minimize any benefits gained by having them.

Paladox added a comment.EditedJan 7 2019, 11:50 PM

Gerrit does not only do 16x16 (it does larger images) and in polygerrit's ui, it can make images look bigger or smaller. It all depends on where you are in the interface, the image is larger on the settings page, but only a bit smaller on the change screen. See https://gerrit-review.googlesource.com/c/gerrit/+/209453

[Phabricator] Conduit is [..] not very fast, so the Gerrit plugin would still have to store images somewhere.

Yeah, anything that queries Phabricator or LDAP in our infra probably shouldn't go without caching. Either Varnish or local Nginx could suffice for that.

That would still exposes a subset of users to the slow request when populating the cache. For that UX performance reason, as well as to eliminate all factors of externally induced load more generally, I recommended earlier to instead have an offline script (e.g. cron) do this periodically. That way, the only thing we expose is a stateless and static file server, with no persistent or versioned storage anywhere. The scripts lets Phabricator generate the thumbs and merely writes them to a working directory. It could do this once an hour or on a loop, controlled by cron via puppet, so either from both puppet or ssh, an admin can disable the script at any time for any reason.

demon added a comment.Jan 8 2019, 5:28 AM

I'm increasingly convinced that avatars aren't worth the effort.

Tgr added a comment.Jan 8 2019, 6:58 AM

Well the effort was inflated by dubious privacy requirements :) I still think the best course of action would be gravatar with a proxy.

Change 545066 had a related patch set uploaded (by Paladox; owner: Paladox):
[operations/puppet@production] Gerrit: Remove All-Avatars from gerrit's module

https://gerrit.wikimedia.org/r/545066

Change 545066 merged by Dzahn:
[operations/puppet@production] Gerrit: Remove All-Avatars from gerrit's module

https://gerrit.wikimedia.org/r/545066

Dzahn added a comment.Oct 22 2019, 7:06 PM

^ The reason to merge this was not a comment on the general question to enable avatars. The reason was that during T222391 we noticed an undesirable dependency. During a Gerrit migration to a new server the existing code meant puppet would try to git pull from avatars repo, fail to do so and then not create the needed Apache site config due to a dependency. So "if avatars repo is not reachable then we don't get Apache config for gerrit.wikimedia.org itself". We had to manually copy config files from the old host to fix that.

hashar lowered the priority of this task from Medium to Low.Oct 23 2019, 9:28 AM

Adding the few project tags we are using nowadays.

Lowering priority since clearly we have no bandwith to work on adding avatar support.

Tgr added a comment.Oct 23 2019, 4:30 PM

Lowering priority since clearly we have no bandwith to work on adding avatar support.

I note this is a self-inflicted problem: the gravatar plugin would work out of the box, and the privacy issues raised before are over-exaggerated, to put it mildly (since the email addresses are already public).

I've come around to agreeing with @Tgr. Gravatar seems like something we could support in good conscience.

The company operating it (Automattic) is one we've dealt with before and wouldn't mind referring to in documentation around setting up your avatar.

Traffic-wise we'd want to proxy it of course, but that seems quite do-able. We could also set a low backend timeouts in Varnish (connect_timeout, first_byte_timeout, between_bytes_timeout) to enforce a strict SLA. Then fail-fast with 404, 503, or even with a static placeholder response (e.g. inline synthetic response with 1px transparent gif).

Krinkle removed a subscriber: Krinkle.Apr 17 2020, 5:03 PM

Change 615514 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] ATS: remove gerrit.wmfusercontent.org

https://gerrit.wikimedia.org/r/615514

Change 615557 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/dns@master] remove gerrit.wmfusercontent.org

https://gerrit.wikimedia.org/r/615557

Change 615514 merged by Dzahn:
[operations/puppet@production] ATS: remove gerrit.wmfusercontent.org

https://gerrit.wikimedia.org/r/615514

Change 615557 merged by Dzahn:
[operations/dns@master] remove gerrit.wmfusercontent.org

https://gerrit.wikimedia.org/r/615557