One last point to note is that GitLab currently isn't open to non-WMF/NDA users and so limits the possibility of external/volunteer involvement.

The libraries were consolidated in mediawiki/libs/metrics-platform but that repo structure doesn't play nicely with the package managers we'll be relying on for distribution, which expect the relevant metadata files (composer.json, Package.swift, etc.) in the repo root.

Not sure if I was supposed to say something here first, but I've just created four new repos in GitLab to host the individual Metrics-Platform-Planning client libraries.

Calling this resolved as we just merged a lot of code relying on the new config syntax.

In T291200#7361096, @Urbanecm wrote:

Another option would be to investigate why are arbcom wikis in wikipedia.dblist and remove if no reason is found

I'll take the opportunity to link the task about this that you mentioned the other day: T183549: Arbcom wikis are in both wikipedia.dblist and special.dblist

To memorialize more fully what happened here:

To expand on my dislike of the reliance on ordering: Before I realized that PHP associative arrays were ordered and would support the behavior currently relied upon, I started out by splitting out the pattern-based stream configs out into their own dedicated config setting, and I still think that would be a good idea for performance in its own right. The implementation could first see if a key matching the requested name exists in the (now-string-keyed) $wgEventStreams, and if not found, iterate over the elements of $wgEventStreamsByPattern to see if any match by regex pattern. Then searches for pattern matches would only have to iterate over a small number of elements (currently only 2: /^swift\.(.+\.)?upload-complete$/ and /^mediawiki\\.job\\..+/) rather than a substantial chunk of the large and growing full list in $wgEventStreams.

Here's a pleasant PHP surprise: associative (i.e., string-keyed) arrays in PHP are ordered, so we don't have to choose between keying by stream name/pattern and having stream configs defined in a preferred ordering. In fact, as this patch demonstrates, it would be totally safe to update $wgEventStreams today to be keyed by stream name/pattern with no adverse effect on its functioning.

See also: https://blog.pragmaticengineer.com/swift-the-only-modern-language-with-no-mocking-framework/

A note on the Swift library: my testing strategy for the core MetricsClient object in PHP, JS, and Java has been based on mocking. This is more difficult for the Swift library because AFAIK there is no mocking library currently in existence that's compatible with a pure Swift project. I've looked at https://github.com/birdrides/mockingbird and https://github.com/Brightify/Cuckoo and I don't believe either will work without substantial modifications.

Hey @Jgiannelos, your proposed event schema looks good to me. That said, I think it belongs in the primary event schema repository as an event that (quoting the README) "directly affect[s] user-facing features."

After discussing this with Toni I decided to drop the library and just implement this using the standard Codable/CodingKeys approach, which is a bit more boilerplate code but is working great. That change is reflected in the lastest patch set.

Current status on the WIP patch: the SerializedSwift library for serializing objects to/from JSON had seemed to be working well, but I've found a bug in it that needs to be resolved if we want to more forward with it. The bug is that null values are explicitly serialized as { "property": null } rather than being omitted, which is the default in GSON, which inspired SerializedSwift, and which is what we want. I believe it's a bug because the intention in the code is pretty clearly that these values be omitted.

This is core functionality of the client libraries for the v1 release. I'm going to be bold and close it as I don't think it needs its own ticket.

Ship sailed.

Work continues apace: https://gerrit.wikimedia.org/r/q/project:mediawiki%252Flibs%252Fmetrics-platform+status:open

Does T272238: Elasticsearch and Kibana are switching to non-OSI-approved SSPL licence affect whether we want to move forward with this?

Background. Everything old is new again:
T102079: Metrics about the use of the Wikimedia web APIs
T114017: Map current use of Wikimedia web APIs

OK, I finally figured out why we're seeing these default discrepancies. The web preferences are being enabled in a hook handler: https://github.com/wikimedia/mediawiki-extensions-Echo/blob/15c72fe4ed1f071af8524a10508f182c5dc48702/includes/EchoHooks.php#L22-L75. I should have known. This should be easy enough to fix.

In T287909#7293188, @JMinor wrote:

As for the solution, I think the "ask for both web and apps" is the second alternative I was making sure would make sense.

It's also an option for the client to opt in to push notifications for all supported types when the user first registers a token, or for a new type when support for a new type is added. (That gets into the surprise avoidance issues I mentioned earlier, but I'm done fighting that battle; do whatever you think is right.)

In T287909#7293036, @JMinor wrote:

The impact however, is that now you can only see a notification on the app's notification center if and only if you also agree to push that notification type.

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?

Coverage reporting is in place for all libraries. Now we just need a target ;)

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.

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 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.

I'm going to tag Prod Infra with this, and add @dr0ptp4kt as EM. I'm not really officially supporting this stuff anymore, just trying to help out.

About the defaulting behavior, after some code investigation and further thinking on the matter: it does not appear to be possible at present to set an entire notifier type to be opt-out. The recommended default, and the assumption throughout the notification preferences code, is for notifications to be opt-in, and the code for determining a user's eligibilty for receiving a notification is already complex enough that I would hesitate to add conditional user preference defaults by notifier type to the mix.

@JMinor Changing that default will only take a config change. I'd say we can do it as part of this task.

Per https://www.mediawiki.org/wiki/Manual:SameSite_cookies#Browser_warnings it looks like these warnings can be ignored, as we have no need to send the cookies in question in any cross-domain AJAX requests.