I don't think we can just add the variable without violating the policy.

The simplest thing to do here is probably to continue exposing the IP. Right now these functions check the user_name variable but that variable can be anything:

To QA this:

I was working on the other observability task and happened to notice this upon some cursory investigation. Leaving this here in case it's useful for whoever picks this up (possibly me next week). It seems like the code is sensible and should emit to express_router_request_duration_seconds.service.ipoid, as the return of app.metrics.getServiceLabel() is {service: 'ipoid' }. This isn't making it to Prometheus and looking at the values.yaml helmfile, we don't specify a Prometheus endpoint. Maybe that's what we need? config.prod.yaml suggests these values:

there could be a situation where they decided to send an IPv6 in a specific format but then not used this format when parsing the response.

In T354758#9605053, @CodeReviewBot wrote:

stran opened https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/237

Return IPs in format user uses

Chatted with Kosta 1:1 and I'm a fan of removing dependencies wherever possible and pushed up a patch implementing the IPUtils approach.

Discussed this with @kostajh @Daimona @Dreamy_Jazz @Tchanders and we came to the consensus that it would be ideal if we could just create the temporary account on the action even though there won't be any edits associated with the account, as it maintains the stance that all actions should be associated with some sort of account. We were hoping to clear this with DBAs, as it's possible that accounts added here would never be valid end users (eg. LTAs triggering filters). I am vaguely aware of some discussion of table bloat but don't know where it is specifically so I'm not sure what the constraints are.

I curled the server and it looks like it's still up, which I think supports the log delay theory. Could this something intermittent like T357616: Logs from containers sometimes not visible in logstash?

I filed the additional tasks T357772: Investigate: How will `ip_in_range` and `ip_in_ranges` function when temporary accounts are enabled and T357774: Investigate: What to do with existing filters that temporary accounts will break to capture the rest of the work noted by my investigation and I've put every task as a child of the parent T307060: [Epic] Temporary account AbuseFilter support so it can be tracked under that umbrella. I'm going to call this task done for now but please re-open if you disagree. 🙇

Somewhat related, but this question came up during the broad AbuseFilter investigation and it seems relevant.

Okay, here are some summaries of my research and technical proposals. I don't think my conclusions here encompass everything that needs to be done but hopefully it's enough to get us going in the right direction.

Going to decline this for the same reason as T356182: TypeError: Cannot read properties of undefined (reading 'feed_file_yesterday'). Please feel free to re-open if anyone disagrees.

I'm going to decline this. Even if I added a guard against this situation, it would fail when attempting to run the main command because --yesterday 20240109 is mismatched against the real state of the database (empty). In a situation where import_status is empty, then the expected command would be an initial import.

If we are including a line about the data source (as per design) then perhaps we can link to the Spur documentation?

It seems like the feed file can be updated over the course of the day. I happened to have an older copy of the feed file and downloaded a new copy while investigating and noticed that my file was 1.01GB and the new file was 1.02GB. Checking the old file vs the new file, I saw that the IP exists in the new (and presumably now-static) file but wasn't in the feed that the db probably used:

% grep {IP} 20240202.new.json

This is because it's trying to pass '' value for the yesterday feed and falls in the same category of error as T356182: TypeError: Cannot read properties of undefined (reading 'feed_file_yesterday'), in which the initial import was expected to succeed and therefore the script doesn't try to accommodate it. However, Invalid date passed. Date should be in yyyymmdd format. has been a useless debugging line for months because it doesn't give any indication what value was passed that's being rejected so we might as well fix it here as well as write in some graceful failures.

This is expected behavior and is incredibly poorly documented and we should probably put this somewhere more obvious than a comment in a test file (import-state.test.js):

I don't know how likely this is to come up on production.

What kinds of errors are we guarding against?

https://gitlab.wikimedia.org/repos/mediawiki/services/ipoid/-/merge_requests/212/diffs should solve this too