Page MenuHomePhabricator

Make kartotherian error logging more verbose
Closed, ResolvedPublic

Description

This is a second round of improvements to error logging. In T321164 we found that nothing was logged for a particular type of error, and found some places in the code where errors might fall through without logging.

  • Log errors regardless of their "metrics" payload. Currently we skip logging under the assumption that the metrics will include the error, but statsd is insufficient for debugging.
  • Exceptions from mapdata are not wired yet. These should show up in the main kartotherian log, at level "error".
    • Attach failure information to each group or to request.
    • Remove our previous attempt which logged from mapdata in many different promise rejection blocks, and allow the errors to bubble up to fail a single group.
    • Kartographer mapdata consumers should handle per-group and per-batch failures.
    • kartotherian mapdata consumer should handle per-group and per-batch failures.
    • Fully test new behavior.
    • This spun out a whole universe of "refactor mapdata".
      • Tests to cover happy and unhappy paths for old code.
      • Only dedupe once. There were several pieces of logic all intending to "deduplicate" mapdata requests by returning data already fetched. This is trivial, we can remove the complex code to support concurrent requests and map keys can be dropped in most places, using flat lists instead.
      • Simplify promises. For example, in many places we wrap promises in another ad-hoc flavor, proxying the resolve and reject callbacks, etc.
      • Drop class hierararchy
      • Flatten geodata recursion: only certain combinations are supported. Simplify and tighten up so that only these structures are followed.
        • Top-level mapdata can be an object or an array of objects.
        • Each top-level object might include ExternalData which needs to be fetched.
      • Split dependency injection from query parameters. These are different functions. DI is stored in context variables, query parameters are passed through with each function call.
      • Nice to have: move "attribution" render to Kartographer
      • Follow-up task: Collect all mapdata requests for a page and request in one query.
  • Release mapdata 0.8 to npm and upgrade consumers.

Details

SubjectRepoBranchLines +/-
mediawiki/services/kartotherianmaster+1 -3
mediawiki/services/kartotherianmaster+8 -4
mediawiki/extensions/Kartographermaster+363 -683
mediawiki/services/kartotherianmaster+37 -37
mapdatamaster+179 -701
mapdatamaster+4 -46
mapdatamaster+27 -10
mapdatamaster+4 -61
mapdatamaster+19 -28
mapdatamaster+155 -22
mediawiki/services/kartotherianmaster+3 -5
mediawiki/services/kartotherianmaster+5 -1
mapdatamaster+92 -0
mapdatamaster+92 -14
mapdatamaster+2 -3
mapdatamaster+101 -22
mapdatamaster+240 -141
mapdatamaster+5 -25
mapdatamaster+36 -5
mediawiki/services/kartotherianmaster+4 -2
mediawiki/services/kartotherianmaster+143 -120
mapdatamaster+0 -15
mapdatamaster+18 -54
mediawiki/extensions/Kartographermaster+0 -3
mapdatamaster+54 -110
mapdatamaster+31 -125
mediawiki/extensions/Kartographermaster+7 -44
mapdatamaster+0 -21
mediawiki/services/kartotherianmaster+6 -6
mediawiki/services/kartotherianmaster+90 -3
mapdatamaster+23 -7
mapdatamaster+120 -27
mapdatamaster+14 -1
mapdatamaster+20 -44
mediawiki/services/kartotherianmaster+4 -2
mediawiki/services/kartotherianmaster+1 -1
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 854489 merged by jenkins-bot:

[mediawiki/services/kartotherian@master] Unknown type *what*?

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

Change 854964 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Catch errors from each group and log

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

Change 854965 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] [WIP] Always allow errors to bubble

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

Change 854558 abandoned by Awight:

[mapdata@master] [WIP] Allow errors to bubble up, log at the top level

Reason:

Has been split

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

Change 854995 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] [WIP] Test unhappy paths

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

Change 854964 merged by jenkins-bot:

[mapdata@master] Catch errors from each group and log

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

Change 854995 merged by jenkins-bot:

[mapdata@master] Test some unhappy paths

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

Change 855606 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Integration test for DataManager

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

Change 855606 merged by jenkins-bot:

[mapdata@master] Integration test for DataManager

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

Change 856519 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/services/kartotherian@master] Make the feature unrolling code testable

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

Change 856921 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/services/kartotherian@master] Skip unnecessary properties normalization

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

Change 856922 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/services/kartotherian@master] Test flattenArraysAndFeatureCollections

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

Change 856923 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/services/kartotherian@master] Clarify mapdata GeoJSON errors

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

Change 856954 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Drop optional debounce behavior

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

Change 856968 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] [WIP] Drop deduplication logic

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

Change 856975 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Kartographer@master] Remove unreachable code

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

Change 856976 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Drop "load" method

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

Change 856986 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] [WIP] more depromisification

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

Change 856922 merged by jenkins-bot:

[mediawiki/services/kartotherian@master] Test flattenArraysAndFeatureCollections

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

Change 856923 merged by jenkins-bot:

[mediawiki/services/kartotherian@master] Clarify mapdata GeoJSON errors

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

Change 856976 abandoned by Awight:

[mapdata@master] Drop "load" method

Reason:

The assumptions were invalid. For an example of the "jsondata.data" attribute in use: https://www.mediawiki.org/w/api.php?action=jsondata&format=json&formatversion=2&title=Neighbourhoods%2FNew+York+City.map

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

Change 856975 abandoned by Awight:

[mediawiki/extensions/Kartographer@master] Remove unreachable code

Reason:

Invalid. Example of jsondata.data in action: https://www.mediawiki.org/w/api.php?action=jsondata&format=json&formatversion=2&title=Neighbourhoods%2FNew+York+City.map

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

Change 858566 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] [WIP] Drop class hierarchy and simplify promises

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

Change 856968 abandoned by Awight:

[mapdata@master] [WIP] Drop deduplication logic

Reason:

Squashed into Ieab409a19105a8532ae8445d3f42e35cb71b71bb

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

Change 856986 abandoned by Awight:

[mapdata@master] [WIP] more depromisification

Reason:

Squashed into Ieab409a19105a8532ae8445d3f42e35cb71b71bb

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

Change 858599 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@master] Removed unnecessary debounce option from mapdata lib call

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

Change 858599 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Removed unnecessary debounce option from mapdata lib call

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

Change 854965 merged by jenkins-bot:

[mapdata@master] Always allow errors to bubble

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

Change 856954 merged by jenkins-bot:

[mapdata@master] Drop optional debounce behavior

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

Change 856519 merged by jenkins-bot:

[mediawiki/services/kartotherian@master] Rewrite GeoJSON Feature unrolling code

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

Change 859092 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/services/kartotherian@master] [WIP] Log failed groups from the consumer

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

Change 859434 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Move title and revid into method call

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

Change 859442 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Include failureReason for each group

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

Change 859443 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Don't log from this library

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

Change 859469 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Extract ExternalData parse into a specialized class

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

Change 859480 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Extract ExternalData fetch into a specialized class

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

Change 859533 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Kartographer@master] Don't need group ID

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

Change 858622 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/services/kartotherian@master] [WIP] Use the new mapdata signatures

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

Change 859442 merged by jenkins-bot:

[mapdata@master] Include failureReason for each group

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

Change 859443 merged by jenkins-bot:

[mapdata@master] Don't log from this library

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

Change 859469 merged by jenkins-bot:

[mapdata@master] Extract ExternalData parse into a specialized class

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

Change 859480 merged by jenkins-bot:

[mapdata@master] Extract ExternalData fetch into a specialized class

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

Change 861828 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Fix documentation of recursive entrypoints

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

Change 861828 merged by jenkins-bot:

[mapdata@master] Fix documentation of recursive entrypoints

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

Change 861857 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/services/kartotherian@master] Error if no shapes are found

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

Change 861858 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/services/kartotherian@master] Bubble up error

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

Change 861899 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Finish ExternalData parser tests

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

Change 861899 abandoned by Awight:

[mapdata@master] Finish ExternalData parser tests

Reason:

squashed

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

Change 862244 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Tests for DataManager.load

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

Change 862244 merged by jenkins-bot:

[mapdata@master] Tests for DataManager.load

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

Change 862268 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Pull tests forward

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

Change 862849 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] [WIP] Drop attribution

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

Change 863227 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/services/kartotherian@master] Log failed external data URL if available

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

awight removed awight as the assignee of this task.Dec 2 2022, 8:20 AM
awight updated the task description. (Show Details)
awight moved this task from Doing to Tech Review on the WMDE-TechWish-Sprint-2022-11-29 board.

Change 863227 abandoned by Awight:

[mediawiki/services/kartotherian@master] Log failed external data URL if available

Reason:

squashed

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

Change 861858 abandoned by Awight:

[mediawiki/services/kartotherian@master] Bubble up error

Reason:

squashed

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

Note to reviewers, here's a mixed map of inline data and external data, which should show only a marker. To make the external data succeed, remove the "foo" prefix.

<mapframe width="300" height="400" zoom="10">
[
  {
    "type": "Feature",
    "properties": {
       "marker-symbol": "town-hall", 
       "marker-color": "46ea5f", 
       "marker-size": "medium"
    },
    "geometry": { 
      "type": "Point",
      "coordinates": [-73.9903, 40.6928] 
    }
  },
  {
    "type": "ExternalData",
    "service": "page",
    "title": "fooNeighbourhoods/New York City.map"
  }
]
</mapframe>

Change 863235 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Clarify error when mapdata cannot be fetched

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

Change 862268 merged by jenkins-bot:

[mapdata@master] Pull tests forward

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

Change 858566 merged by jenkins-bot:

[mapdata@master] Drop class hierarchy and simplify promises

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

Change 859434 merged by jenkins-bot:

[mapdata@master] Move title and revid into method call

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

Change 862849 merged by jenkins-bot:

[mapdata@master] Drop attribution

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

Change 863235 merged by jenkins-bot:

[mapdata@master] Clarify error when mapdata cannot be fetched

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

Change 866328 had a related patch set uploaded (by Awight; author: Awight):

[mapdata@master] Update README to new API

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

Change 866328 merged by jenkins-bot:

[mapdata@master] Update README to new API

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

Change 858622 merged by jenkins-bot:

[mediawiki/services/kartotherian@master] Breaking upgrade: mapdata

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

The last branch of 2022 is cut, so we're free to merge the Kartographer changes and test on the beta cluster.

Change 859533 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Breaking upgrade: mapdata

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

awight moved this task from Tech Review to Done on the WMDE-TechWish-Sprint-2023-01-04 board.

The final patch is merged and can be tested on the beta cluster. I was able to load mapdata and externaldata through the updated library, we can let the code go out via next week's train.

There's nothing to demo for this backend work.

This should have landed on production with this week's train. I don't see any artifacts in Grafana corresponding to the deployment dates, so it seems that no damage was done!

Change 856921 abandoned by WMDE-Fisch:

[mediawiki/services/kartotherian@master] Skip unnecessary properties normalization

Reason:

This part of the code got removed in I2b8ed087a57c33af595dd8ba735971b0ca4d7201

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