Page MenuHomePhabricator

AQS 2.0: resolve inconsistencies between Page Analytics and Device Analytics
Closed, ResolvedPublic

Description

There are a few minor differences between Device Analytics (our first target for production deployment) and Page Analytics. Resolving these will make future code maintenance easier.

  • Device Analytics has a configuration/configuration.go file Page Analytics calls this file configuration/config.go. Note: kask and service-scaffold-golang both have a similar file in the root directory, by the name config.go. It seems that Device Analytics is the outlier here, probably when we introduced the "configuration" package. Was there a reason we went with "configuration" and not "config"? If we choose "configuration", we should consider (later, not now) pushing that change back up to the scaffolding level.
  • The Device Analytics version of the configuration allows setting various things about the Cassandra connection (via a "cassandra" struct). We should add that to Page Analytics
  • There are some other minor unnecessary differences between the files. It'd be good to diff them and make them as consistent as possible. That'll make it easier, if we're debugging something related to config that works in one service but not another. It'll also make it easier if we decide to extract config from the individual services and move it up to a higher level (aqsassist, servicelib, etc.).
  • In similar fashion, there are minor unnecessary differences in not_found_handler.go

Event Timeline

SGupta-WMF changed the task status from Open to In Progress.May 8 2023, 9:09 AM
SGupta-WMF claimed this task.
SGupta-WMF edited projects, added AQS2.0 (Sprint 10); removed AQS2.0.

@BPirkle I noticed in device-analytics , not-found-handler is still in main package , that needs to be updated in device-analytics. I think , we can do it along with T336216 .

@BPirkle I noticed in device-analytics , not-found-handler is still in main package , that needs to be updated in device-analytics. I think , we can do it along with T336216 .

Good catch. That plan works for me.

Change 919472 had a related patch set uploaded (by Sg912; author: Sg912):

[generated-data-platform/aqs/page-analytics@main] Resolving inconsistencies between device-analytics and page-analytucs

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

Change 919472 merged by BPirkle:

[generated-data-platform/aqs/page-analytics@main] Resolving inconsistencies between device-analytics and page-analytucs

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

As noted on the patch, I'm getting one itest error. But I was previously getting one (different) test error, so this seems like at worst a lateral move, and I wanted to get this large-ish change merged before it started accumulating merge conflicts again. If the itest error is a QA blocker, we can consider it under a different patchset.

Here's the error I saw:

--- FAIL: TestPerArticle (0.05s)
    --- FAIL: TestPerArticle/returns_expected_data_for_monthly_granularity (0.00s)
        per_article_test.go:157:
            	Error Trace:	/Users/bpirkle/go/src/page-analytics/itest/per_article_test.go:157
            	Error:      	"[]" should have 1 item(s), but has 0
            	Test:       	TestPerArticle/returns_expected_data_for_monthly_granularity
            	Messages:   	Invalid number of results

@SGupta-WMF , do you see that too, or is it just me? Should I advance this task to QA as-is and give @EChukwukere-WMF the expectation that this error will be resolved in a separate task and is not a QA blocker, or would you like to look at this error first under this task?

@BPirkle Yes , this test was always failing even when it was in test package . This will be resolved in this task T336158