Add Persistent Activity Logger #2730

nelsonkopliku · 2024-07-01T11:50:51Z

Description

This PR introduces the ability to track interesting activities in the system.
Initial focus in on tracking http related activities.

Notes:

metadata extraction testing has been omitted in PhoenixConnParserTest but tested in ActivityLogger which covers the broader integration of the different components
in order to not make the PR even bigger only tests about login activity and tagging/untagging have been added in ActivityLogger. More test about the other interesting activities to follow.

EDIT: Since we currently do not need different kind of destinations where to write logs, a few abstractions have been removed.

How was this tested?

automated tests.

gagandeepb · 2024-07-03T13:32:57Z

lib/trento/activity_logging/registry.ex

+  @login_attempt {TrentoWeb.SessionController, :create}
+  @api_key_generation {TrentoWeb.V1.SettingsController, :update_api_key_settings}
+  @saving_suma_settings {TrentoWeb.V1.SUMACredentialsController, :create}


Maybe for later: Use the OpenAPI spec as a single source of truth for this purpose.

Interesting idea. Let's indeed keep that on top of our minds as dealing with openapispex schemas would be a beast on its own 😄

lib/trento/activity_logging/registry.ex

gagandeepb · 2024-07-03T13:36:12Z

lib/trento/activity_logging/phoenix_conn_parser.ex

@@ -0,0 +1,127 @@
+defmodule Trento.ActivityLogging.Logger.PhoenixConnParser do


Could this potentially be a Protocol?

I did not think about that, yet. I cannot firmly say yes or no at the moment.
I guess when we introduce the Commanded related activity logging we might discover what it needs to be.

gagandeepb

Looks fine, for the most part. Up to you how/when you want to address the comments.

gagandeepb · 2024-07-08T08:09:43Z

lib/trento/activity_logging/activity_catalog.ex

+defmodule Trento.ActivityLog.ActivityCatalog do
+  @moduledoc """
+  Activity logging catalog
+  """
+
+  @type logged_activity :: {controller :: module(), activity :: atom()}
+
+  @login_attempt {TrentoWeb.SessionController, :create}
+  @api_key_generation {TrentoWeb.V1.SettingsController, :update_api_key_settings}


This introduces a boundary issue with the Trento name space now depending on TrentoWeb (usually it is the other way around). One way to resolve this would be to create a top-level namespace ActivityLog for all related modules, especially the ones with cross-cutting module dependencies. WDYT ?

boundary issue with the Trento name space now depending on TrentoWeb

I hear you, I see what you mean and I tend to agree.

Consider we already break that dependency direction in projectors, enriching commanded middleware and in an amqp message processor (mainly views and TrentoWeb.Endpoint in order to broadcast things via websocket) 🙈

Now, I am not saying that since we broke some rule we should ruthlessly keep doing so 😄
What I mean is that we accepted the trade-off of having access to TrentoWeb components within Trento in certain edgy areas (like projectors and what mentioned earlier).

All the references to TrentoWeb controllers are kept inside the centralized Trento.ActivityLog.ActivityCatalog (meant to be like that) and I would keep accepting this trade-off also for this feature, unless until it matures more.

I will track an item with the concern you're raising.

Solution wide, I guess you mean having a top level ./lib/activity_log (or similar) next to ./lib/trento and ./lib/trento_web. It might make sense indeed, as it would be a cross-cutting concern, as you mentioned.

Gonna track this in the backlog as well.
Thanks!

What if we just move the ActivityCatalog to the web app? It may make sense as it refers to controller operations.

🤔 we'd be needing to access TrentoWeb.{WhateverNamespace}.ActivityCatalog from within Trento and we'd be back in the same situation.

Besides, the activity catalog does belong to activity logging better than web, imho, because it would possibly reference also non-web related activity.

lib/trento/activity_logging/activity_catalog.ex

gagandeepb · 2024-07-08T08:52:07Z

lib/trento/activity_logging/parser/activity_parser.ex

+defmodule Trento.ActivityLog.Parser.ActivityParser do
+  @moduledoc """
+  Behavior for activity parsers.
+  It extracts the activity relevant information from the context.
+  """
+
+  alias Trento.ActivityLog.ActivityCatalog
+
+  @callback detect_activity(activity_context :: any()) :: ActivityCatalog.logged_activity() | nil
+
+  @callback get_activity_actor(
+              activity :: ActivityCatalog.logged_activity(),
+              activity_context :: any()
+            ) :: any()
+
+  @callback get_activity_metadata(
+              activity :: ActivityCatalog.logged_activity(),
+              activity_context :: any()
+            ) :: map()
+end


defprotocol Trento.ActivityLog.ParseToActivity do alias Trento.ActivityLog.ActivityCatalog def detect_activity(activity_context :: any()) :: ActivityCatalog.logged_activity() | nil def get_activity_actor( activity :: ActivityCatalog.logged_activity(), activity_context :: any() ) :: any() def get_activity_metadata( activity :: ActivityCatalog.logged_activity(), activity_context :: any() ) :: map() end

For reference/ consideration. I think this should work.

gagandeepb · 2024-07-08T08:54:41Z

lib/trento/activity_logging/parser/phoenix_conn_parser.ex

@@ -0,0 +1,140 @@
+defmodule Trento.ActivityLog.Logger.Parser.PhoenixConnParser do


For example:

defimpl Trento.ActivityLog.ParseToActivity, for: %Plug.Conn{} do ... end

Thanks for the tips on the protocols, might come good as we proceed further

lib/trento/infrastructure/activity_log/logger/adapter/database_writer.ex

gagandeepb

Looks good. LGTM.

…om an activity context

…ity logs to a destination

…ttp connections

nelsonkopliku · 2024-07-10T08:58:00Z

@gagandeepb FYI since we currently do not need different kind of destinations where to write logs, a few abstractions have been removed and the overall implementation slightly simplified.

We can add them later if and when needed.

nelsonkopliku self-assigned this Jul 1, 2024

nelsonkopliku added enhancement New feature or request user story elixir Pull requests that update Elixir code labels Jul 1, 2024

nelsonkopliku force-pushed the activity-logger branch from f0105fb to ed215f1 Compare July 1, 2024 12:41

Base automatically changed from activity-logger-plug to main July 1, 2024 12:53

nelsonkopliku force-pushed the activity-logger branch 4 times, most recently from 16c8a8a to d2420f0 Compare July 3, 2024 08:01

gagandeepb reviewed Jul 3, 2024

View reviewed changes

nelsonkopliku force-pushed the activity-logger branch 2 times, most recently from 5f2113b to 4fa886d Compare July 4, 2024 12:00

nelsonkopliku requested a review from balanza July 4, 2024 12:01

nelsonkopliku marked this pull request as ready for review July 4, 2024 12:02

nelsonkopliku added the env Create an ephimeral environment for the pr branch label Jul 4, 2024

gagandeepb approved these changes Jul 8, 2024

View reviewed changes

nelsonkopliku force-pushed the activity-logger branch from 4fa886d to 5f654fe Compare July 8, 2024 12:26

gagandeepb approved these changes Jul 8, 2024

View reviewed changes

nelsonkopliku added 11 commits July 10, 2024 09:44

Adjust ActivityLogger.log_activity signature

a5487de

Add an ActivityLog catalog determining what is of interest to be logged

f8774d2

Add an ActivityParser behaviour that extracts relevant information fr…

609164e

…om an activity context

Add an ActivityLogWriter behaviour that is responsible to write activ…

414f443

…ity logs to a destination

Add a PhoenixConnParser able to parse/extract activity from phoenix h…

2f69716

…ttp connections

Add a DatabaseWriter that writes activity logs to the database

abb0353

Add a Persistent activity logger

18146f3

Adjust configuration

8a2c8cb

Define custom activity_type type

81508f0

Alias Trento Repo in Database log writer

4196b89

Refactor activity logging

fb1db88

nelsonkopliku force-pushed the activity-logger branch from 95cb859 to fb1db88 Compare July 10, 2024 07:44

Defer activity log writer abstraction

80339b8

nelsonkopliku merged commit 727e866 into main Jul 10, 2024
26 checks passed

nelsonkopliku deleted the activity-logger branch July 10, 2024 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Persistent Activity Logger #2730

Add Persistent Activity Logger #2730

nelsonkopliku commented Jul 1, 2024 •

edited

Loading

gagandeepb Jul 3, 2024

nelsonkopliku Jul 4, 2024

gagandeepb Jul 3, 2024

nelsonkopliku Jul 4, 2024

gagandeepb left a comment

gagandeepb Jul 8, 2024

nelsonkopliku Jul 8, 2024

balanza Jul 8, 2024

nelsonkopliku Jul 9, 2024

gagandeepb Jul 8, 2024

gagandeepb Jul 8, 2024

nelsonkopliku Jul 8, 2024

gagandeepb left a comment

nelsonkopliku commented Jul 10, 2024

		@@ -0,0 +1,127 @@
		defmodule Trento.ActivityLogging.Logger.PhoenixConnParser do

		@@ -0,0 +1,140 @@
		defmodule Trento.ActivityLog.Logger.Parser.PhoenixConnParser do

Add Persistent Activity Logger #2730

Add Persistent Activity Logger #2730

Conversation

nelsonkopliku commented Jul 1, 2024 • edited Loading

Description

How was this tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagandeepb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagandeepb left a comment

Choose a reason for hiding this comment

nelsonkopliku commented Jul 10, 2024

nelsonkopliku commented Jul 1, 2024 •

edited

Loading