Seems the deprecation warning stems from this https://gerrit.wikimedia.org/r/c/mediawiki/core/+/666519

Convenient links:

• The task attached to the MediaWiki core change introducing this deprecation warning: T275531: Make RevisionRecord return a ProperPageIdentity
• The two changes Daniel mentioned on IRC:
  • RevisionStore: create getPage, deprecate getTitle
  • RevisionStore: getRevisionbyId should take a PageIdentity

I also notice that MediaWiki core has again hard-deprecated code that is still used in Wikimedia-maintained code even though the stable interface policy forbids this:

Hard deprecation MUST NOT be applied to code still used in Wikimedia maintained code. Such usage MUST be removed first.

Last time this happened, the explanation (T273622#6797449) was that it was not possible to catch these usages without hard deprecation. Maybe this is a similar case, but is it too much to ask that such deprecations are at least called out or explained somewhere, e.g. in the Gerrit change introducing them or in the related Phabricator task? How are developers supposed to flag up these occurrences if the deprecation message doesn’t even mention the related task?

Mentioned in SAL (#wikimedia-operations) [2021-03-16T20:00:48Z] <brennen> 1.36.0-wmf.35 train status (T274939): blocked at group0 on T277362

In T277362#6916991, @Lucas_Werkmeister_WMDE wrote:

I also notice that MediaWiki core has again hard-deprecated code that is still used in Wikimedia-maintained code even though the stable interface policy forbids this:

Hard deprecation MUST NOT be applied to code still used in Wikimedia maintained code. Such usage MUST be removed first.

Last time this happened, the explanation (T273622#6797449) was that it was not possible to catch these usages without hard deprecation. Maybe this is a similar case, but is it too much to ask that such deprecations are at least called out or explained somewhere, e.g. in the Gerrit change introducing them or in the related Phabricator task? How are developers supposed to flag up these occurrences if the deprecation message doesn’t even mention the related task?

That's not a valid reason to bypass the deprecation policy. In the past we just added logging for it (e.g. T176526: Remove $wgTitle fallback from EditPage in MW1.36) until we were satisfied we had caught everything. I would propose reverting the core change on the basis that it did not follow the deprecation policy, and then move the train forward again. Holding it because someone else broke the policy doesn't make sense to me.

Change 672810 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@master] Revert "Deprecate constructing revision with non-proper page"

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

Change 672811 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/core@wmf/1.36.0-wmf.35] Revert "Deprecate constructing revision with non-proper page"

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

I also notice that MediaWiki core has again hard-deprecated code that is still used in Wikimedia-maintained code even though the stable interface policy forbids this

This was of course not intentional - the policy asks for hard deprecation to happen as soon as all callers have been fixed. Which of course in practice means as soon as we think all callers have been fixed. The problem is that it's sometimes hard to find such code - and sometimes it's only found when it starts logging deprecation warnings in production.

That's not a valid reason to bypass the deprecation policy. In the past we just added logging for it (e.g. T176526: Remove $wgTitle fallback from EditPage in MW1.36) until we were satisfied we had caught everything.

Technically, deprecation warnings are logging. I can see that it would be preferable to start out with a lower log level, so these don't show up as production errors. But using "soft" logging means these issues don't show up as test failures. The fact that deprecation warnings make tests fail is very helpful for finding any cases we missed by looking at the code.

Perhaps the solution is to handle deprecation warnings separate from other production issues?

How are developers supposed to flag up these occurrences if the deprecation message doesn’t even mention the related task?

That is a good point - we have not included task IDs in deprecations so far. But perhaps it would be a good idea to do so.

Anyway, the "proper" fix done, I'll clean up the remaining issues in the tests tomorrow: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/672499

For context - the description is of T275531 is indeed rather short, and the real background is in T273284. The idea is that we to avoid data corruption such as T260485, we have to ensure that cross-wiki operations on entities such as users, pages, and revisions are safe. We do that by asserting that they belong to the correct wiki. Title is not cross-wiki aware, so using it in a cross-wiki context was deprecated.

I agree that it was impossible to discover this idea from the deprecation message, and in this case it was also not discoverable from the task linked in the commit message.

I propose we think about how we can minimize disruption by improving how we log deprecation messages, and improve communication by making the relevant information more discoverable in context. Please let me know if you have ideas.

Change 672810 merged by jenkins-bot:
[mediawiki/core@master] Revert "Deprecate constructing revision with non-proper page"

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

Change 672811 merged by jenkins-bot:
[mediawiki/core@wmf/1.36.0-wmf.35] Revert "Deprecate constructing revision with non-proper page"

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

Mentioned in SAL (#wikimedia-operations) [2021-03-16T23:56:32Z] <krinkle@deploy1002> Synchronized php-1.36.0-wmf.35/includes/Revision/: I8619ab9e92b, T277362, T275531 (duration: 00m 58s)

In T277362#6919814, @daniel wrote:

That's not a valid reason to bypass the deprecation policy. In the past we just added logging for it (e.g. T176526: Remove $wgTitle fallback from EditPage in MW1.36) until we were satisfied we had caught everything.

Technically, deprecation warnings are logging. I can see that it would be preferable to start out with a lower log level, so these don't show up as production errors. But using "soft" logging means these issues don't show up as test failures. The fact that deprecation warnings make tests fail is very helpful for finding any cases we missed by looking at the code.

It just pushes the problem onto someone else, by stopping their development and forcing them drop everything just to unbreak their repo - in this case it hit everyone by stopping the entire train. That Wikimedia-deployed code was still using this hard deprecated code was identified on Saturday, once that identification was done, why did it have to rise all the way to a train blocker on Tuesday to get a revert?

In T277362#6919927, @Legoktm wrote:

It just pushes the problem onto someone else, by stopping their development and forcing them drop everything just to unbreak their repo

I was not aware this broke any tests in any repo - did it?

My understanding was that there are no automated tests for the cross-wiki case in Wikibase. Because there are no such tests, the issue did not show up in CI. (Not blaming anyone but myself - I originally wrote the cross-wiki revision loading code in Wikibase, and then I rewrote it for RevisionStore).

I have been wanting to improve the testing situation for quite a while. Perhaps incidents like this one can help to to get more attention and resources for improving our testing platform. Some relevant tickets: T261848: Simulate databases for sister sites in phpunit, T248683: Create and run a suite of end-to-end tests for the Wikimedia environment, T195932: Test all MediaWiki tarball extensions in gate for all changes to MediaWiki and each other.

• in this case it hit everyone by stopping the entire train.

I don't see why deprecation warnings should stop the train at all. I agree that they should be worked on with high priority, but why should they be treated as production errors?

That Wikimedia-deployed code was still using this hard deprecated code was identified on Saturday, once that identification was done, why did it have to rise all the way to a train blocker on Tuesday to get a revert?

I learned about the issue from Hoo on IRC at 17:30 on Monday. I immediately started work on a fix. On Tuesday, that fix got a couple of rounds of review, getting the tests right wasn't trivial. However, the urgency wasn't clear at all, I didn't hear from the Wikidata team all day.

In T277362#6920735, @daniel wrote:

In T277362#6919927, @Legoktm wrote:

It just pushes the problem onto someone else, by stopping their development and forcing them drop everything just to unbreak their repo

I was not aware this broke any tests in any repo - did it?

My understanding was that there are no automated tests for the cross-wiki case in Wikibase. Because there are no such tests, the issue did not show up in CI. (Not blaming anyone but myself - I originally wrote the cross-wiki reloading code in Wikibase, and then I rewrote it for RevisionStore).

It is indeed very unfortunate that there has not been automated tests for this functionality. It would have given the issue more visibility and faster.
WMDE has created tests for this recently -- this is how we identified the problem on Monday -- but he have not yet integrated it into Wikimedia Jenkins CI. I expect us to do this in a couple of weeks.

• in this case it hit everyone by stopping the entire train.

I don't see why deprecation warnings should stop the train at all. I agree that they should be worked on with high priority, but why should they be treated as production errors?

If blocking the train on this issue -- which, BTW, got ultimately triggered by T277593 - WMDE has been the whole day going back and forth in internal chats whether this is a train blocker or whether all will be fine when the code lands in production -- was too extreme, it maybe would be good to have some clarity on how those deprecation warnings, and the exceptions/errors logged stemming from them should be treated. In my eyes treating them as a train blocker has been in line with the recently announced "new line on logspam".

That Wikimedia-deployed code was still using this hard deprecated code was identified on Saturday, once that identification was done, why did it have to rise all the way to a train blocker on Tuesday to get a revert?

I learned about the issue from Hoo on IRC at 17:30 on Monday. I immediately started work on a fix. On Tuesday, that fix got a couple of rounds of review, getting the tests right wasn't trivial. However, the urgency wasn't clear at all, I didn't hear from the Wikidata team all day.

Point taken, we should have been more active in communication about this problem outside of WMDE (virtual) office. Something we shall improve in the future.

In T277362#6920796, @WMDE-leszek wrote:

It is indeed very unfortunate that there has not been automated tests for this functionality. It would have given the issue more visibility and faster.
WMDE has created tests for this recently -- this is how we identified the problem on Monday -- but he have not yet integrated it into Wikimedia Jenkins CI. I expect us to do this in a couple of weeks.

Oh, that's excellent! I'm curious about what approach you took for these tests.
Having them in CI would have prevented to offending code from being merged.

If blocking the train on this issue -- which, BTW, got ultimately triggered by T277593 - WMDE has been the whole day going back and forth in internal chats whether this is a train blocker or whether all will be fine when the code lands in production -- was too extreme, it maybe would be good to have some clarity on how those deprecation warnings, and the exceptions/errors logged stemming from them should be treated. In my eyes treating them as a train blocker has been in line with the recently announced "new line on logspam".

I generally agree with the stricter line on logspam. But I do think that halting the train for deprecation warnings does more harm than good. Perhaps @brennen and @thcipriani can chime in on that.

Generally, halting/reverting deployments should be done to prevent damage to the site. Halting/reverting on logspam seems like a desperate measure, intended to make sure that warnings in production are actually prioritized. Which shouldn't be necessary, but I suppose it is.

In this case however, the warning was clearly harmless (deprecation warnings indicate that things are still working properly), and the issue was already being worked on. Halting the train just caused more work for more people.

I guess part of the problem was that my fix wasn't tagged with this ticket. Which I didn't know about until late last night.

Point taken, we should have been more active in communication about this problem outside of WMDE (virtual) office. Something we shall improve in the future.

I notice that neither this task nor T277593 is on any PET board, even though it became quickly clear that the cause was in core. Pinging the relevant teams and components (in this case, MediaWiki-Core-Revision-backend and Platform Engineering) helps getting more attention on this more quickly, and to coordinate on the way forward.

A handful of responses:

1. Deployers have no way to know in the moment that something is only a deprecation warning with no other consequences. If a warning isn't material to production health, it should probably either be labeled in a way that makes that extremely clear, or be sent to some channel that isn't surfaced where deployers are looking for signs of production breakage.
   
   1a. What is obvious to MediaWiki developers is frequently not obvious to train deployers. We aren't MediaWiki developers.

2. If there's ever a conversation about whether something might become a train blocker, RelEng would greatly appreciate if we were looped into the discussion by having it surfaced on the blocker task. Please see Deployments/Risky change template. We can and will improve our documentation about this. (See T273802.)

3. It is undeniably true that we sometimes halt the train on things which are "only" logspam. Partly this is because it's hard for us to tell the difference without developer input. Partly this is because halting the train is the only way we can get enough of them fixed to keep production logspam down to tolerable levels. If you think this is a broken system, you are probably right, but please remember that our incentives include maintaining a viable signal about production health week over week and month over month. A process which is currently manual in the extreme, and centralized on our team. We're working on changing that, but it's a hard problem.

Thank you for your response, @brennen!

In T277362#6922054, @brennen wrote:

1. Deployers have no way to know in the moment that something is only a deprecation warning with no other consequences. If a warning isn't material to production health, it should probably either be labeled in a way that makes that extremely clear, or be sent to some channel that isn't surfaced where deployers are looking for signs of production breakage.

In my mind, deprecation warnings should never be material to production health. They exist in order to warn before things break. So perhaps they should be channeled elsewhere. But we would have to make sure that they are still noticed, and bugs still get filed. They should definitely not be ignored or allowed to pile up.

2. If there's ever a conversation about whether something might become a train blocker, RelEng would greatly appreciate if we were looped into the discussion by having it surfaced on the blocker task. Please see Deployments/Risky change template. We can and will improve our documentation about this. (See T273802.)

Duly noted. We had a couple of such risky patches a while ago, around ParserCache. We did not think of this patch here as risky, precisely because it only introduced warnings, and did not break behavior.

3. It is undeniably true that we sometimes halt the train on things which are "only" logspam. Partly this is because it's hard for us to tell the difference without developer input. Partly this is because halting the train is the only way we can get enough of them fixed to keep production logspam down to tolerable levels.

I'm not complaining that it's broken, I'm thinking about ways to improve it. And there are certainly different kinds of "warnings" - e.g. a "notice" about accessing an undefined array key may have very serious implications, depending on how that value is being used. I'm trying to think of a better channel for things that are known to be non-critical, things that should be fixed asap but are not a danger to the live site. Fixing deprecation warnings isn't just nice to have, it should be high priority. But it's not UBN, and it should not block the train. Maybe a separate log channel would be the way to go.