Page MenuHomePhabricator
Create Task
Maniphest T288853

Migrated Server-side EventLogging events recording http.client_ip as 127.0.0.1
Closed, ResolvedPublic
Actions

Assigned To
Ottomata
Authored By
nettrom_WMF
Aug 13 2021, 6:38 PM
Tags
Referenced Files
None
Subscribers
BTullis
• DAbad
kostajh
• mewoph
mforns
• Mholloway
Milimetric
View All 10 Subscribers

Description

http.client_ip was added to the Growth schemas in T287121. When verifying events flowing in for those schemas, HomepageVisit and ServerSideAccountCreation are recording http.client_ip as 127.0.0.1 for all events. Here's an example query, which returns no rows:

SELECT
    *
FROM event.serversideaccountcreation
WHERE year = 2021
AND month = 8
AND day >= 12
AND http.client_ip IS NOT NULL
AND http.client_ip != "127.0.0.1"

ServerSideAccountCreation and HomepageVisit are run server-side. The other Growth schemas are client-side and do not appear to be affected by this.

Expected behaviour: http.client_ip records the client IP address and associated geolocation information as for other schemas.

Details

SubjectRepoBranchLines +/-
EventBus - Enable x_client_ip_forwarding_enabled for analytics purposesoperations/mediawiki-configmaster+5 -23
Guard against undefined index notice when setting x-client-ipmediawiki/extensions/EventBuswmf/1.38.0-wmf.1+1 -1
Guard against undefined index notice when setting x-client-ipmediawiki/extensions/EventBuswmf/1.38.0-wmf.2+1 -1
CommonSettings-labs.php - test Eventbus x_client_ip_forwarding_enabled in betaoperations/mediawiki-configmaster+22 -0
Guard against undefined index notice when setting x-client-ipmediawiki/extensions/EventBusmaster+1 -1
EventBus - Enable x_client_ip_forwarding_enabled for analytics purposesoperations/mediawiki-configmaster+5 -1
Support x_client_ip_forwarding_enabled setting per event servicemediawiki/extensions/EventBusmaster+50 -36
Customize query in gerrit

Related Objects

Event Timeline

nettrom_WMF renamed this task from Server-side Event Platform events recording http.client_ip as 127.0.0.1 to Migrated Server-side EventLogging events recording http.client_ip as 127.0.0.1 .Aug 13 2021, 6:38 PM
nettrom_WMF created this task.
nettrom_WMF added a project: Growth-Team.Aug 13 2021, 6:52 PM
• mewoph moved this task from Inbox to Needs Discussion on the Growth-Team board.Aug 16 2021, 10:10 PM
• mewoph added subscribers: kostajh, Ottomata, Tgr, • mewoph.EditedAug 16 2021, 10:20 PM

@Tgr @Ottomata @kostajh — is there a token that can be used to join client_ip from a client-side event or does the IP need to be passed to the server-side event somehow? Since this is server to server, having http.client_ip probably doesn't make sense.

Tgr added a comment.Aug 16 2021, 11:14 PM

MediaWiki should have no problem adding the IP to the request. (Due to proxy mechanics, this might differ from the client-side IP in some edge cases but I doubt we care about those.) IMO that should happen on the EventLogging library level, to keep in sync with client-side logging where it also doesn't need to be manually added.

nettrom_WMF added projects: Data-Engineering, Metrics Platform.Aug 17 2021, 2:17 PM

I'm adding Data Engineering and Metrics Platform to get some visibility on this, as @Tgr mentions this should happen at the library level.

Having the geolocation data available is important to answer questions in Product Dept deep dives, which happen monthly. Thus, the need for this data is rapidly increasing, we're hoping to have some insights available for the next one in mid-September. Getting this fixed before the end of the month would help facilitate that.

nettrom_WMF added subscribers: mforns, Milimetric.Aug 17 2021, 2:26 PM

Adding @Milimetric and @mforns so they can follow up on this.

• Mholloway subscribed.EditedAug 17 2021, 2:56 PM

I believe (though I'm not 100% sure) that all WebRequests on the MW production hosts should have an x-client-ip header added by Varnish/ATS, but I don't see that being passed along anywhere in EventLogging or in EventBus (which actually performs the event submission to eventgate for server-side events). So I think the fix here would be to update EventBus::send() to get and send along the x-client-ip header. I'd appreciate a second opinion on this, though.

kostajh added a comment.Aug 17 2021, 3:07 PM
In T288853#7288640, @Mholloway wrote:

I believe (though I'm not 100% sure) that all WebRequests on the MW production hosts should have an x-client-ip header added by Varnish/ATS, but I don't see that being passed along anywhere in EventLogging or in EventBus (which actually performs the event submission to eventgate). So I think the fix here would be to update EventBus::send() to get and send along the x-client-ip header. I'd appreciate a second opinion on this, though.

That sounds right to me, although perhaps EventFactory::createEvent() would make sense, as that's where other metadata is added.

• Mholloway added a comment.EditedAug 17 2021, 5:25 PM

Thanks, @kostajh.

From earlier discussions with @Ottomata, I believe that the top-level http and meta properties are in a sense "owned" by the intake service, and the intent is for this kind of supplementation to happen there. So I think passing along X-Client-IP as an HTTP request header, to allow it to be used as http.client_ip by eventgate if needed, would more closely adhere to internal logic/assumptions than would directly supplementing the event data with it in EventFactory. We can't add http.client_ip to the event body unconditionally, or I believe events will be rejected by eventgate where http.client_ip is not present in the schema; and although eventgate-wikimedia leaves http.client_ip alone if it's present in the schema and a value is already set, I don't think the intent in general is for the metrics client to examine event schemas (as opposed to stream configs) or for producers to set it directly.

I have a patch in progress that I can push for review shortly, but it looks like we've already missed the MW branch cut this week. If it's sufficiently urgent, it can be backported.

gerritbot added a comment.Aug 17 2021, 6:45 PM

Change 713526 had a related patch set uploaded (by Mholloway; author: Michael Holloway):

[mediawiki/extensions/EventBus@master] Conditionally send X-Client-IP header to the intake service

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

gerritbot added a project: Patch-For-Review.Aug 17 2021, 6:45 PM
• Mholloway added a comment.Aug 17 2021, 7:23 PM
In T288853#7289080, @Mholloway wrote:

I believe that the top-level http and meta properties are in a sense "owned" by the intake service, and the intent is for this kind of supplementation to happen there.

Hmm, counterpoint to self: For server-side events, EventBus (via EventFactory) is already directly producing all of the meta fields. OTOH, it's safe to assume the presence of meta in EventBus, because the meta object is included in every schema, while http.client_ip is not.

Tgr added a comment.Aug 17 2021, 9:42 PM

IMO there is value in client-side and server-side events indicating the client IP in an identical way, because it might be used for things other than processing inside the event intake service, e.g. for network-level monitoring and anti-abuse (where it's also helpful that X-Client-IP is a commonly used header and all kinds of tools understand it).

That said, maybe worth asking someone from the Traffic team?

• Mholloway added a project: Traffic.Aug 18 2021, 4:55 PM
In T288853#7289672, @Tgr wrote:

IMO there is value in client-side and server-side events indicating the client IP in an identical way, because it might be used for things other than processing inside the event intake service, e.g. for network-level monitoring and anti-abuse (where it's also helpful that X-Client-IP is a commonly used header and all kinds of tools understand it).

Just to be clear, you're advocating here for getting http.client_ip from X-Client-IP for events produced server-side from MediaWiki, correct? I believe that's what's happening now for client-produced events. (Actually, looking at the relevant code, eventgate looks first to X-Client-IP, then falls back to the leftmost IP in X-Forwarded-For, then finally falls back to req.socket.remoteAddress, though the function doc block only says that the field contains "[the] value of X-Client-IP request header if set."). My guess is that 127.0.0.1 is the req.socket.remoteAddress for requests coming from the MediaWiki appservers.

That said, maybe worth asking someone from the Traffic team?

Tagging Traffic and hoping that gets the right person's attention. :)

Maintenance_bot added a project: SRE.Aug 18 2021, 5:45 PM
Milimetric added a comment.Aug 18 2021, 8:28 PM

I'm just making sure I didn't miss anything: it looks to me like the instrumentation's just not sending the correct client IP. If something else is happening that needs or attention on EventGate or other parts of the pipeline, please let me know but I didn't get that from the discussion above.

In general I agree with Tgr, there's value in keeping the ip as consistent as possible between client-side and server-side.

Tgr added a comment.Aug 18 2021, 9:47 PM
In T288853#7292309, @Mholloway wrote:

Just to be clear, you're advocating here for getting http.client_ip from X-Client-IP for events produced server-side from MediaWiki, correct?

Yes.

BTullis subscribed.Aug 23 2021, 3:17 PM
odimitrijevic triaged this task as High priority.Aug 23 2021, 4:06 PM
odimitrijevic moved this task from Incoming to Event Platform on the Analytics board.
ldelench_wmf moved this task from Triage to Tracking on the Product-Analytics board.Aug 23 2021, 4:13 PM
mforns assigned this task to • Mholloway.Aug 23 2021, 6:04 PM

Just for the record, some discussion has happened in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EventBus/+/713526/.
I believe in the end we're sending the IP via X-Forwarded-For header. And this should work already, because of:

... eventgate looks first to X-Client-IP, then falls back to the leftmost IP in X-Forwarded-For, then finally falls back to req.socket.remoteAddress ...

Thank you @Mholloway et. al. for taking care of this.
I think it makes sense to assign this task to you @Mholloway, please ping us if you need help from Data Engineering.

Ottomata added a comment.Aug 30 2021, 1:40 PM

Hello!

I believe that the top-level http and meta properties are in a sense "owned" by the intake service

No fields are 'owned' by the intake service (AKA EventGate). There are some fields that are 'owned' by client producer libraries, like EventLogging / EventBus. eventgate-wikimedia will set some default values in events if they are not set, but it is easy to do this in EventGate because EventGate has the schema of the event, and can use the schema to determine if a default field should be set.

Schema fields that are not always present (like http.client_ip) cannot be automatically set by client producer libraries because they have no knowledge of the event's schema. The only place to set this now is in the instrumentation producer code, since that code is constructing an event that conforms to a specific schema.

The patch as proposed (forwarding X-Client-IP to EventGate) is certainly interesting though! It would work and I am not opposed, however we don't think that what EventGate is doing (using schemas to determine which defaults to set) is the right thing to do in the long run. https://phabricator.wikimedia.org/T273235#6879386. Instead, we should use stream config to enable/disable event hydrations, since all client producer libraries should be stream config aware.

• jlinehan mentioned this in T290014: Source geolocation directly rather than using IP in schema.Aug 30 2021, 3:40 PM
nettrom_WMF added a comment.Sep 13 2021, 6:52 PM

Hi all, coming back to this as I've been OoO. As mentioned in T288853#7288436, having this data is becoming more urgent with the Product Deep Dives and the Newcomer Pilot. We've been running experiments aiming to increase account registration and activation and get questions about country-level differences that we're unable to answer (they might be possible to answer with various other proxies, but having the data available in ServerSideAccountCreation and HomepageVisit would make things a lot easier).

Is there something I can do to help move this along? @Ottomata, in your last comment you say you're not opposed, however the patch has a -1 on it from you. I get that there might be a preferred long-time solution for this that's different from what the patch proposes, and it looks like T290014 is the task for that as its description says "exploring whether we can source geolocation directly". That sounds like it could take a while, and I'd much prefer a solution that makes data start flowing in tomorrow rather than wait longer for this.

• jlinehan moved this task from Backlog to To be prioritised on the Metrics Platform board.Sep 13 2021, 8:45 PM
kostajh moved this task from Needs Discussion to Triaged on the Growth-Team board.Sep 14 2021, 11:50 AM

Moving this to Growth-Team's triaged column as @Mholloway is working on this; if there is something specific needed from our team beyond review on the patch, please let us know.

Ottomata added a comment.Sep 14 2021, 4:32 PM

Ottomata, in your last comment you say you're not opposed, however the patch has a -1 on it from you

The -1 is not for the idea, but for a specific implementation concern in the patch, I think it'd just need to be resolved.

nettrom_WMF added a comment.Sep 15 2021, 2:47 PM
In T288853#7352497, @Ottomata wrote:

The -1 is not for the idea, but for a specific implementation concern in the patch, I think it'd just need to be resolved.

Got it, thanks for clarifying that!

gerritbot added a comment.Sep 17 2021, 4:24 PM

Change 713526 merged by jenkins-bot:

[mediawiki/extensions/EventBus@master] Support x_client_ip_forwarding_enabled setting per event service

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

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.1; 2021-09-21).Sep 17 2021, 5:00 PM
Maintenance_bot removed a project: Patch-For-Review.Sep 17 2021, 5:10 PM
nettrom_WMF added a comment.Sep 27 2021, 6:09 PM

Looks like this got deployed last week with the train? I'm not seeing any changes in the server-side schemas, they're still recording 127.0.0.1 as the IP address. Maybe that's expected and there are additional changes needed? Let me know if something's needed on my end or if I can help out with something to get this resolved.

Ottomata added a comment.Sep 27 2021, 6:20 PM

We need to enable it per eventgate service. Patch OTW...

gerritbot added a comment.Sep 27 2021, 6:26 PM

Change 724143 had a related patch set uploaded (by Ottomata; author: Ottomata):

[operations/mediawiki-config@master] EventBus - Enable x_client_ip_forwarding_enabled for analytics purposes

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

gerritbot added a project: Patch-For-Review.Sep 27 2021, 6:26 PM
gerritbot added a comment.Sep 27 2021, 6:47 PM

Change 724143 merged by Ottomata:

[operations/mediawiki-config@master] EventBus - Enable x_client_ip_forwarding_enabled for analytics purposes

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

Ottomata added a project: Analytics-Kanban.Sep 27 2021, 6:52 PM
Stashbot added a comment.Sep 27 2021, 6:56 PM

Mentioned in SAL (#wikimedia-operations) [2021-09-27T18:56:53Z] <otto@deploy1002> Synchronized wmf-config/CommonSettings.php: REVERT: Enable x_client_ip_forwarding_enabled for eventgate-analytics and eventgate-analytics-external - T288853 (duration: 00m 56s)

Ottomata added a comment.Sep 27 2021, 7:04 PM

There is a bug in the code, so I had to revert my config patch. Hope to follow up this week.

Maintenance_bot removed a project: Patch-For-Review.Sep 27 2021, 7:11 PM
nettrom_WMF added a comment.Sep 27 2021, 8:04 PM
In T288853#7381469, @Ottomata wrote:

There is a bug in the code, so I had to revert my config patch. Hope to follow up this week.

Thanks for following up on this, appreciate it! I'll keep an eye on this task before I attempt to QA anything.

gerritbot added a comment.Sep 27 2021, 9:14 PM

Change 724180 had a related patch set uploaded (by Ottomata; author: Ottomata):

[mediawiki/extensions/EventBus@master] Guard against undefined index notice when setting x-client-ip

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

gerritbot added a project: Patch-For-Review.Sep 27 2021, 9:14 PM
Ottomata moved this task from Next Up to In Code Review on the Analytics-Kanban board.Sep 27 2021, 9:15 PM
gerritbot added a comment.Sep 28 2021, 12:14 PM

Change 724180 merged by jenkins-bot:

[mediawiki/extensions/EventBus@master] Guard against undefined index notice when setting x-client-ip

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

Maintenance_bot removed a project: Patch-For-Review.Sep 28 2021, 1:11 PM
gerritbot added a comment.Sep 28 2021, 3:23 PM

Change 724451 had a related patch set uploaded (by Ottomata; author: Ottomata):

[operations/mediawiki-config@master] CommonSettings-labs.php - test Eventbus x_client_ip_forwarding_enabled in beta

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

gerritbot added a project: Patch-For-Review.Sep 28 2021, 3:23 PM
gerritbot added a comment.Sep 28 2021, 3:26 PM

Change 724451 merged by Ottomata:

[operations/mediawiki-config@master] CommonSettings-labs.php - test Eventbus x_client_ip_forwarding_enabled in beta

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

Ottomata added a comment.EditedSep 28 2021, 3:54 PM

Tested in beta, looks ok there.

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.3; 2021-10-05); removed MW-1.38-notes (1.38.0-wmf.1; 2021-09-21).Sep 28 2021, 4:00 PM
Ottomata claimed this task.Sep 28 2021, 4:07 PM
Maintenance_bot removed a project: Patch-For-Review.Sep 28 2021, 4:10 PM
gerritbot added a comment.Sep 28 2021, 5:52 PM

Change 724480 had a related patch set uploaded (by Ottomata; author: Ottomata):

[mediawiki/extensions/EventBus@wmf/1.38.0-wmf.2] Guard against undefined index notice when setting x-client-ip

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

gerritbot added a project: Patch-For-Review.Sep 28 2021, 5:52 PM

Change 724481 had a related patch set uploaded (by Ottomata; author: Ottomata):

[mediawiki/extensions/EventBus@wmf/1.38.0-wmf.1] Guard against undefined index notice when setting x-client-ip

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

gerritbot added a comment.Sep 28 2021, 5:56 PM

Change 724380 had a related patch set uploaded (by Ottomata; author: Ottomata):

[operations/mediawiki-config@master] EventBus - Enable x_client_ip_forwarding_enabled for analytics purposes

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

Ottomata added a comment.EditedSep 28 2021, 6:00 PM

Scheduled for a backport window tomorrow (wednesday sept 29).

Ottomata moved this task from In Code Review to Ready to Deploy on the Analytics-Kanban board.Sep 28 2021, 9:00 PM
gerritbot added a comment.Sep 30 2021, 6:34 PM

Change 724480 merged by jenkins-bot:

[mediawiki/extensions/EventBus@wmf/1.38.0-wmf.2] Guard against undefined index notice when setting x-client-ip

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

gerritbot added a comment.Sep 30 2021, 6:34 PM

Change 724481 merged by jenkins-bot:

[mediawiki/extensions/EventBus@wmf/1.38.0-wmf.1] Guard against undefined index notice when setting x-client-ip

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

Stashbot added a comment.Sep 30 2021, 7:04 PM

Mentioned in SAL (#wikimedia-operations) [2021-09-30T19:04:00Z] <thcipriani@deploy1002> Synchronized php-1.38.0-wmf.2/extensions/EventBus/includes/EventBus.php: Backport: [[gerrit:724480|Guard against undefined index notice when setting x-client-ip (T288853)]] (duration: 01m 09s)

Stashbot added a comment.Sep 30 2021, 7:05 PM

Mentioned in SAL (#wikimedia-operations) [2021-09-30T19:05:40Z] <thcipriani@deploy1002> Synchronized php-1.38.0-wmf.1/extensions/EventBus/includes/EventBus.php: Backport: [[gerrit:724481|Guard against undefined index notice when setting x-client-ip (T288853)]] (duration: 01m 09s)

Ottomata added a comment.Sep 30 2021, 7:27 PM

Backport window didn't go yesterday, so I got the bugfixes out today. I'd rather wait until Monday to enable the config change, will do that then.

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.1; 2021-09-21); removed MW-1.38-notes (1.38.0-wmf.3; 2021-10-05).Oct 1 2021, 5:21 PM
• jlinehan moved this task from To be prioritised to Sign-off on the Metrics Platform board.Oct 4 2021, 2:21 PM
• DAbad subscribed.Oct 6 2021, 1:39 PM

@Ottomata can we close this ticket out now? Or is there work left?

Ottomata added a comment.Oct 6 2021, 1:42 PM

There is still work, I haven't deployed the config change. Sorry about that. I got caught up in the DSE hackathon this week and didn't want to have to deal with any fallout.

If you want it sooner, you could schedule the config change for a backport window deployment.

gerritbot added a comment.Oct 12 2021, 1:12 PM

Change 724380 merged by Ottomata:

[operations/mediawiki-config@master] EventBus - Enable x_client_ip_forwarding_enabled for analytics purposes

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

Stashbot added a comment.Oct 12 2021, 1:13 PM

Mentioned in SAL (#wikimedia-operations) [2021-10-12T13:13:38Z] <otto@deploy1002> Synchronized wmf-config/CommonSettings.php: Enable x_client_ip_forwarding_enabled for eventgate-analytics and eventgate-analytics-external - T288853 (duration: 00m 56s)

Ottomata added a comment.Oct 12 2021, 1:16 PM

Okay, deployed, and I see HomepageVisit events with real client IPs.

@nettrom_WMF, f it looks okay to you, we can resolve.

nettrom_WMF closed this task as Resolved.Oct 12 2021, 4:46 PM

I've verified that there are now events in the Data Lake with client IPs and geolocation information as expected. Thanks @Mholloway and @Ottomata for your work on getting this bug fixed!

nettrom_WMF mentioned this in T306018: Instrument blocked account registration.Aug 10 2022, 4:16 PM
nettrom_WMF mentioned this in T368495: client_ip attribute reports only 127.0.0.1 in PHP/API context.Jun 26 2024, 3:21 PM
Maintenance_bot removed a project: Patch-For-Review.Jun 26 2024, 3:31 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
  NODES
Idea 6
idea 6
INTERN 1
Note 7
Project 18
Verify 1