Page MenuHomePhabricator
Create Task
Maniphest T325633

Investigate: Log and store errors
Closed, ResolvedPublic13 Estimated Story Points
Actions

Description

In general, we should be able to 1. log errors as they happen and 2. possibly be paged by emergent critical errors. Please investigate:

  • If we get any of this logging out of the box with service-runner (the repo security-api is forked from)
  • If not, how we should implement this logging

This may deserve its own ticket, but while importing data I noticed a few lines failing without any additional details. Presumably, the data was in a format that the import script didn't expect but without being able to log the failing data, there was no way to determine next steps to fixing it. If as part of this investigate there's an easy way to implement this logging, please either do so or file an additional ticket if it's more complicated than expected.

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
WIP Log errors with database writes from init-db.jsmediawiki/services/ipoidmaster+16 -1
Customize query in gerrit

Related Objects

Event Timeline

STran created this task.Dec 20 2022, 1:26 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 20 2022, 1:26 PM
STran mentioned this in T325640: Investigate: Implement error handling.Dec 20 2022, 2:05 PM
Niharika set the point value for this task to 13.Dec 20 2022, 5:28 PM
Tchanders claimed this task.May 17 2023, 3:39 PM
Tchanders edited projects, added Anti-Harassment-Team (AHaT Sprint 30: Avanton Gold Cone); removed Anti-Harassment-Team.
Tchanders subscribed.

Do we get error logging out of the box?

Yes.

Example:

  • I forced an error on this line by returning undefined from feedUtil.createConnection
  • I then checked out http://localhost:6927/feed/v1/ip/185.186.83.70
    • An error was displayed: {"status":500,"type":"internal_error","title":"TypeError","detail":"Cannot read property 'query' of undefined","method":"GET","uri":"/feed/v1/ip/185.186.83.70"}
    • This was logged (viewable via docker log <container ID>), including the stack trace: "stack":"TypeError: Cannot read properties of undefined (reading 'query')\n at /srv/service/routes/feed/v1.js:32:29\n at processTicksAndRejections (node:internal/process/task_queues:96:5)"

How it's working:

  • Error handling is implemented in lib/util.js
  • app.js calls setErrorHandler from lib/util.js, which logs the error and formats the response body (I tinkered with the error/response formatting to prove this)

What about outside the web app?

  • None of the above applies to the data import pipeline, or web app setup
  • I tried forcing the same error as above from the data import pipeline, and the error was logged with a helpful stack trace: "TypeError: Cannot read properties of undefined (reading 'query') at addTables (/srv/service/init-db.js:22:22) [...]"
  • I tried forcing an error during web startup by introducing a typo, and again the error was logged with a helpful stack trace: "stack":"ReferenceError: expressssssss is not defined\n at initApp (/srv/service/app.js:24:17)\n [...]"

@STran Was this what you wanted to know?

This may deserve its own ticket, but while importing data I noticed a few lines failing without any additional details. Presumably, the data was in a format that the import script didn't expect but without being able to log the failing data, there was no way to determine next steps to fixing it.

Could you provide any more details about these, or are they addressed by the errors you were fixing recently?

Tchanders moved this task from Ready 🎬 (ONLY IF YOU HAVE NO MORE CODE TO REVIEW) to In Progress 💪 on the Anti-Harassment-Team (AHaT Sprint 30: Avanton Gold Cone) board.May 17 2023, 3:39 PM
STran added a comment.May 20 2023, 10:14 AM

@STran Was this what you wanted to know?

@Tchanders When I wrote this ticket, I thought it would be possibly be good to allow errors to fail and be logged but not halt the entire import since it's 20+M lines and we'd have to redo it from the beginning since there isn't a concept of resuming built into the app. I thought those errors could be logged for a human to check out (eg. the data coming in is bad and we should ignore it or there was an error in import and we should import it, manually or otherwise).

Could you provide any more details about these, or are they addressed by the errors you were fixing recently?

I've fixed the ones I found but that doesn't guarantee more won't show up. Some of the possible errors are described in and will be fixed in https://gerrit.wikimedia.org/r/c/wikimedia/security/security-api/+/921036 (the ipoid version of this). As mentioned, if more show up for whatever reason, it would be good if a human could double check it.

For instance, that patch increases the org column to 1280 characters not because I thought any valid org would have a name that long, but it covers this entry (which, upon any sort of human inspection, is invalid):

The activity you have detected originates from a dynamic hosting environment. For fastest response, please submit abuse reports at http://aws-portal.amazon.com/gp/aws/html-forms-controller/contactus/AWSAbuse For more information regarding EC2 see: http://ec2.amazonaws.com/ All reports MUST include: * src IP * dest IP (your IP) * dest port * Accurate date/timestamp and timezone of activity * Intensity/frequency (short log extracts) * Your contact details (phone and email) Without these we will be unable to identify the correct owner of the IP address at that point in time.

I would prefer not to increase the org character limit to that size and let this pass through but did it as part of my fixes so that I could parse the rest of the dataset, since any error would kill the process.

gerritbot added a comment.May 30 2023, 3:59 PM

Change 924552 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/services/ipoid@master] WIP Log errors with database writes from init-db.js

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

gerritbot added a project: Patch-For-Review.May 30 2023, 3:59 PM
Tchanders mentioned this in T338016: Catch errors when inserting data for individual IP addresses into the database.Jun 2 2023, 10:27 AM
Tchanders moved this task from In Progress 💪 to Done Q4 2022-2023 on the Anti-Harassment-Team (AHaT Sprint 30: Avanton Gold Cone) board.Jun 2 2023, 11:29 AM
In T325633#8866840, @STran wrote:

@STran Was this what you wanted to know?

@Tchanders When I wrote this ticket, I thought it would be possibly be good to allow errors to fail and be logged but not halt the entire import since it's 20+M lines and we'd have to redo it from the beginning since there isn't a concept of resuming built into the app. I thought those errors could be logged for a human to check out (eg. the data coming in is bad and we should ignore it or there was an error in import and we should import it, manually or otherwise).

Thanks @STran. This should be captured by T338016, and the patch above (or its equivalent in Gitlab) is tagged against that task instead.

gerritbot added a comment.Jun 2 2023, 11:30 AM

Change 924552 abandoned by Tchanders:

[mediawiki/services/ipoid@master] WIP Log errors with database writes from init-db.js

Reason:

Moved to GitLab: https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/3

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

Maintenance_bot removed a project: Patch-For-Review.Jun 2 2023, 12:10 PM
STran moved this task from Backlog to Done on the iPoid-Service board.Jun 16 2023, 8:31 AM
Tchanders closed this task as Resolved.Jun 20 2023, 3:23 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits