PP-495 Analytics as a container service #1487

RishiDiwanTT · 2023-10-30T12:28:36Z

Description

Using the dependency_injector framework, the Analytics class has been moved into a Container as a Singleton.
The Analytics no longer supports per library configurations, any configuration is for the entire CM.
Configurations have been moved to environment variables.
The API objects also make use of the dependency injection pattern for analytics.
The Admin UI for the Analytics service will display a "This service is no longer configurable" message.

Database entries for Analytics have not been removed in case a rollback is required.

Motivation and Context

The CM no longer needed per-library Analytics, it required only CM-wide configurations, not via the Admin Panel.
JIRA

How Has This Been Tested?

Manually tested some feeds and Admin pages.
Updated all unit tests to reflect the new patterns.

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

Analytics is no more a singleton, and is not library dependant Analytics will be configured through env vars now and not the admin ui

The GET method returns a problem detail for the UI to display

… is created only once

codecov · 2023-10-30T12:38:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dca7f61) 90.42% compared to head (4d0f59b) 90.37%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1487      +/-   ##
==========================================
- Coverage   90.42%   90.37%   -0.05%     
==========================================
  Files         231      232       +1     
  Lines       29513    29330     -183     
  Branches     6892     6852      -40     
==========================================
- Hits        26687    26507     -180     
- Misses       1798     1800       +2     
+ Partials     1028     1023       -5

Flag	Coverage Δ
Api	`73.69% <88.46%> (-0.09%)`	⬇️
Core	`61.00% <90.19%> (+0.06%)`	⬆️
migration	`29.29% <72.54%> (+0.76%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
api/admin/controller/__init__.py	`100.00% <ø> (ø)`
api/admin/controller/base.py	`79.51% <ø> (-0.95%)`	⬇️
api/admin/controller/view.py	`95.91% <100.00%> (-0.17%)`	⬇️
api/admin/routes.py	`95.01% <ø> (-0.12%)`	⬇️
api/discovery/registration_script.py	`90.62% <100.00%> (ø)`
api/s3_analytics_provider.py	`68.62% <100.00%> (+1.46%)`	⬆️
core/analytics.py	`100.00% <100.00%> (+2.43%)`	⬆️
core/feed/annotator/circulation.py	`90.84% <100.00%> (+0.06%)`	⬆️
core/local_analytics_provider.py	`100.00% <100.00%> (ø)`
core/metadata_layer.py	`87.62% <100.00%> (+0.02%)`	⬆️
... and 6 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tdilauro · 2023-10-30T15:26:31Z

I haven't started a code review here, but it looks there will be some deployment changes associated with this PR (I've added a label for that). Specifically, it looks like we add at least the following environment variables:

~~PALACE_LOCAL_ANALYTICS_ENABLED (default: False)~~
PALACE_S3_ANALYTICS_ENABLED (default: False)
~~PALACE_LOCATION_SOURCE (default: None, other values: ["neighborhood"])~~

It would be helpful to document these in the README.

api/admin/routes.py

api/axis.py

api/circulation.py

api/controller.py

core/service/analytics/configuration.py

tests/api/test_bibliotheca.py

api/controller.py

core/feed/annotator/circulation.py

jonathangreen · 2023-10-30T16:27:09Z

I added the UI update label as well, since this will require an admin UI update.

jonathangreen

Just a few more comments on this one. Its looking good otherwise!

We also need two additional PRs before this can get merged:

Add the new env var to the hosting-playbook
Remove the analytics configuration from the admin interface

Can you work on those as well?

jonathangreen · 2023-11-01T12:21:36Z

api/admin/routes.py

@@ -484,22 +484,6 @@ def metadata_service_self_tests(identifier):
    )


-@app.route("/admin/analytics_services", methods=["GET", "POST"])


Can you make a PR to the admin interface to remove the analytics service component on that side as well?

jonathangreen · 2023-11-01T12:24:07Z

README.md


 For generating quicksight dashboard links the following environment variable is required
 `QUICKSIGHT_AUTHORIZED_ARNS` - A dictionary of the format `"<dashboard name>": ["arn:aws:quicksight:...",...]`
 where each quicksight dashboard gets treated with an arbitrary "name", and a list of "authorized arns".
 The first the "authorized arns" is always considered as the `InitialDashboardID` when creating an embed URL
 for the respective "dashboard name".

+#### Analytics
+
+To enabled S3 based analytics add `PALACE_S3_ANALYTICS_ENABLED=true` else do not add the environment variable at all.


Suggested update:

Local analytics are enabled by default. S3 analytics can be enabled via the following environment variable:

PALACE_S3_ANALYTICS_ENABLED: A boolean value to disable or enable s3 analytics. The default is false.

core/local_analytics_provider.py

core/metadata_layer.py

core/service/configuration.py

tests/api/test_bibliotheca.py

jonathangreen · 2023-11-01T12:51:16Z

tests/api/test_bibliotheca.py

@@ -1564,10 +1599,10 @@ def test_end_to_end(
        # book, and one to the metadata endpoint for information about
        # that book.
        api = default_monitor.api


Minor: If you wanted to avoid the # type: ignore [attr-defined] comments here, you could do:

api = cast(MockBibliothecaAPI, default_monitor.api)

api/controller.py

Since we inject the Analytics provider directly, this is unneeded

RishiDiwanTT · 2023-11-02T07:00:41Z

Just a few more comments on this one. Its looking good otherwise!

We also need two additional PRs before this can get merged:

Add the new env var to the hosting-playbook

Remove the analytics configuration from the admin interface

Can you work on those as well?

Sure, I'll make tickets for them and get started.

jonathangreen

This looks good!

Before this gets merged, we should make sure that:

PR for hosting playbook is merged and deployed
PR for admin UI is merged

So that everything stays in sync.

RishiDiwanTT added 3 commits October 30, 2023 15:37

Analytics as a service through dependency injection

17adaf4

Analytics is no more a singleton, and is not library dependant Analytics will be configured through env vars now and not the admin ui

Removed the admin UI analytics service controller

0a131d3

The GET method returns a problem detail for the UI to display

Added an analytics container with a singleton so the analytics object…

d69c203

… is created only once

RishiDiwanTT requested a review from a team October 30, 2023 12:28

RishiDiwanTT self-assigned this Oct 30, 2023

RishiDiwanTT changed the title ~~Analytics as a container service~~ PP-495 Analytics as a container service Oct 30, 2023

Mypy fixes

8912c16

tdilauro added the Deployment Changes Requires changes to deployment configuration. label Oct 30, 2023

jonathangreen reviewed Oct 30, 2023

View reviewed changes

jonathangreen added the UI Update This PR requires an admin UI update label Oct 30, 2023

RishiDiwanTT added 5 commits October 31, 2023 12:03

Switched to the injection pattern for CirculationManager load_settings

8a17b6d

Removed the analytics requirement from the licensing methods

7d89c5d

Inject s3 service directly in the the analytics provider

3984b21

Removed location_source and adjoining logic

d63cc7c

Moved away from container_isntance() towards injection

7c4fb18

RishiDiwanTT requested a review from jonathangreen October 31, 2023 11:50

RishiDiwanTT added 4 commits October 31, 2023 17:23

Removed unused variables

064ff50

Removed the admin analytics route

98c537c

Injected analytics into the overdrive new title script

7c76a10

Typing fixes

6e85231

jonathangreen reviewed Nov 1, 2023

View reviewed changes

RishiDiwanTT added 2 commits November 2, 2023 11:51

Review comments update

d037323

Removed the Serivces container from the CirculationManager

4d0f59b

Since we inject the Analytics provider directly, this is unneeded

RishiDiwanTT requested a review from jonathangreen November 3, 2023 11:34

jonathangreen approved these changes Nov 3, 2023

View reviewed changes

RishiDiwanTT merged commit 93928ee into main Nov 6, 2023
30 checks passed

RishiDiwanTT deleted the feature/analytics-service branch November 6, 2023 14:10

jonathangreen added the incompatible changes Changes that require a new major version label Nov 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PP-495 Analytics as a container service #1487

PP-495 Analytics as a container service #1487

RishiDiwanTT commented Oct 30, 2023 •

edited

Loading

codecov bot commented Oct 30, 2023 •

edited

Loading

tdilauro commented Oct 30, 2023 •

edited

Loading

jonathangreen commented Oct 30, 2023

jonathangreen left a comment

jonathangreen Nov 1, 2023

jonathangreen Nov 1, 2023

jonathangreen Nov 1, 2023

RishiDiwanTT commented Nov 2, 2023

jonathangreen left a comment

		@@ -484,22 +484,6 @@ def metadata_service_self_tests(identifier):
		)


		@app.route("/admin/analytics_services", methods=["GET", "POST"])

PP-495 Analytics as a container service #1487

PP-495 Analytics as a container service #1487

Conversation

RishiDiwanTT commented Oct 30, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Oct 30, 2023 • edited Loading

Codecov Report

tdilauro commented Oct 30, 2023 • edited Loading

jonathangreen commented Oct 30, 2023

jonathangreen left a comment

Choose a reason for hiding this comment

jonathangreen Nov 1, 2023

Choose a reason for hiding this comment

jonathangreen Nov 1, 2023

Choose a reason for hiding this comment

jonathangreen Nov 1, 2023

Choose a reason for hiding this comment

RishiDiwanTT commented Nov 2, 2023

jonathangreen left a comment

Choose a reason for hiding this comment

RishiDiwanTT commented Oct 30, 2023 •

edited

Loading

codecov bot commented Oct 30, 2023 •

edited

Loading

tdilauro commented Oct 30, 2023 •

edited

Loading