-
- Notifications
You must be signed in to change notification settings - Fork 743
Initial conversion of Test Kitchen client library to Kotlin. #6179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
dr0ptp4kt left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbrant looks okay. I left a couple non-blocker comments, at least I think they're probably non-blockers - your call upon your reflection.
If you choose to do a small change on the event queue size for the calling context of TK event submission I reckon that can be done and self-merged if you think it fit.
I left a comment about specifying the streams of interest when calling into the Action API for the listing; there I think if you did more advanced collation of the streams in use I'd recommend the other teammates look at least at your strategy for collation of the stream names for submission. A possible problem if collating is your URL may get too large, which would bring you back to just fetching all the streams. Standardizing on use of of the pre-ordained schema fields, which would be some more involved work, can in the future help you switch to not being able to pass too many stream names on the URL should you find that something of interest, I suppose!
Happy holidays Dmitry!
| | ||
| @GET(MW_API_PREFIX + "action=streamconfigs&format=json&constraints=destination_event_service=eventgate-analytics-external") | ||
| suspend fun getStreamConfigs(): MwStreamConfigsResponse | ||
| @GET(MW_API_PREFIX + "action=streamconfigs&format=json&constraints=destination_event_service%3Deventgate-analytics-external") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unimportant in the context of this here PR, but you can also pass a streams parameter and have a pipe-separated list of streams (e.g., streams=product_metrics.app_base|<stream2>|.... It quietly ignores non-present streams if at least one of the stream names is valid. Normally I might suggest as a build step to fetch the fully crafted URL and bake that into the app build. And then confirm at runtime that, following normalization of the JSON for the Action API call to action=streamconfigs, there's conformance about the aspects of each stream (it's certainly conceivable there could be changes, possibly introduced by a platform maintainer or someone doing cleanup, that would make the freshest version of the stream differ from what you would have baked into the app build; and that may make one reluctant to go down this path of looking for consistency). But, that's boiling a larger pot of water than you probably want to deal with right now...although I can see how maybe you might be interested in shrinking the payload of the JSON response, at least, supposing you can collate all of the streams that are in use in the app (maybe via some registry callback, I guess). I'd trust if you do that in this PR as an additional commit that you could have direct teammates check for comprehensiveness of the list.
| private fun addToEventQueue(event: Event?) { | ||
| var eventQueueAppendAttempts = max(eventQueue.size / 50, 10) | ||
| | ||
| if (eventQueue.size > queueCapacity / 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is intentional, but this in practice seems to be a big number whereby event send won't be terribly likely until there's been a lot of action happening for the TK-facilitated events (although app lifecycle events in practice may result in event emission). Do we normally want to enqueue so many messages before sending? I guess it may be true that we have a fair amount of instrumentation, so possibly future state this is actually what we want. But, perhaps no harm in letting this send a bit more freely.
Consider this comment non-blocking. I trust your judgement on what do here. In case you do make it more aggressively send, feel free to touch in place and self merge. It does actually work.
kafkacat -C -b kafka-jumbo1010.eqiad.wmnet:9092 -t eqiad.android.product_metrics.article_link_preview_interaction -o 0 | grep -I Boer {"$schema":"/analytics/product_metrics/app/base/1.6.0","dt":"2025-12-20T13:51:25.65Z","meta":{"stream":"android.product_metrics.article_link_preview_interaction","id":"1938a2b5-c12f-4ad1-affe-b761f481e492","dt":"2025-12-20T13:53:21.575Z","request_id":"d8645aba-c24f-4701-9940-cdd2cca4938c"},"agent":{"app_flavor":"proddebug","app_install_id":"04b49646-03bf-4f49-adba-0d111b15d94b","app_theme":"LIGHT","app_version":564,"app_version_name":"WikipediaApp/50564-r-2025-12-20","client_platform":"android","client_platform_family":"app","device_family":"google sdk_gphone64_arm64","device_language":"en","release_status":"prod"},"page":{"id":6000895,"title":"The_Great_Boer_War","namespace_id":0,"wikidata_qid":"Q3751494","content_language":"en"},"mediawiki":{"database":"enwiki"},"performer":{"is_logged_in":false,"session_id":"011fc62b143a8498ad06","pageview_id":"3163e4abafbcac26ece1","groups":[],"language_groups":"[en]","language_primary":"en"},"action":"linkclick","action_source":"2","action_context":"{\"time_spent_ms\":5649}","sample":{"unit":"device"},"http":{"request_headers":{"user-agent":"WikipediaApp/50564-r-2025-12-20 (Android 16; Phone; sdk_gphone64_arm64 Build/BE2A.250530.026.D1) Google Play"},"client_ip":"<REDACTED>"}} % Reached end of topic eqiad.android.product_metrics.article_link_preview_interaction [0] at offset 4203254075 {"$schema":"/analytics/product_metrics/app/base/1.6.0","dt":"2025-12-20T13:52:10.841Z","meta":{"stream":"android.product_metrics.article_link_preview_interaction","id":"8246c802-99ff-4d1b-b9bd-2b7dcf54a341","dt":"2025-12-20T13:53:21.575Z","request_id":"d8645aba-c24f-4701-9940-cdd2cca4938c"},"agent":{"app_flavor":"proddebug","app_install_id":"04b49646-03bf-4f49-adba-0d111b15d94b","app_theme":"LIGHT","app_version":564,"app_version_name":"WikipediaApp/50564-r-2025-12-20","client_platform":"android","client_platform_family":"app","device_family":"google sdk_gphone64_arm64","device_language":"en","release_status":"prod"},"page":{"id":6000895,"title":"The_Great_Boer_War","namespace_id":0,"wikidata_qid":"Q3751494","content_language":"en"},"mediawiki":{"database":"enwiki"},"performer":{"is_logged_in":false,"session_id":"011fc62b143a8498ad06","pageview_id":"3163e4abafbcac26ece1","groups":[],"language_groups":"[en]","language_primary":"en"},"action":"cancel","action_source":"2","action_context":"{\"time_spent_ms\":50907}","sample":{"unit":"device"},"http":{"request_headers":{"user-agent":"WikipediaApp/50564-r-2025-12-20 (Android 16; Phone; sdk_gphone64_arm64 Build/BE2A.250530.026.D1) Google Play"},"client_ip":"<REDACTED>"}}
This is an initial conversion of the "latest" version of the Metrics Platform Java client library to Kotlin, and will now be included as a Module in our app project.
Currently, this library will work exactly as it would have done, under its logic of Metrics Platform and its associated schema, which is still able to ingest events.
This can be considered an initial step towards more complete integration with the present state of Test Kitchen, which is the successor to Metrics Platform.
All package names now have "testkitchen" nomenclature, even though the internal logic still betrays its Metrics Platform roots.
https://phabricator.wikimedia.org/T401023