Skip to content

Analytics update events#91

Merged
lsipii merged 40 commits intomainfrom
feat/analytics-update-events
Nov 13, 2023
Merged

Analytics update events#91
lsipii merged 40 commits intomainfrom
feat/analytics-update-events

Conversation

@lsipii
Copy link
Copy Markdown
Contributor

@lsipii lsipii commented Oct 17, 2023

Adds some metrics for the use of monitoring dashboard

  • relates to: Extend the primary dashboard features monitoring#30
  • metrics introduced:
    • on request runtime:
      • description: metrics published with CloudWatch client at every request using AnalyticsService.cs
      • metrics:
        • RequestsTotal: total requests (should match with the lambda invocations count)
        • RequestsPerContext: request counts by request context identifier eg. "{httpMethod} {requestPath}"
        • RequestsTotalPerAudience: how many requests by an application (af, keha, etc)
        • RequestsTotalPerIssuer: requests by authentication provider (issuer)
    • as background task:
      • description: metrics gathered from the database and then published to CloudWatch as a lambda function task: UpdateAnalyticsAction.cs
      • invoked from:
        • manually using admin function action: "UpdateAnalytics"
        • CloudWatch Events scheduler: once a day
        • SQS queue trigger initiatied by AnalyticsService:
          • triggered after profiles being created or deleted
      • metrics:
        • PersonsCount: how many profiles in total
        • PersonsCountByIssuer: by auth provider
        • PersonsCountByAudience: by application

lsipii added 30 commits October 16, 2023 13:42
… refactor the admin actions dependencies as injected
@lsipii lsipii marked this pull request as ready for review October 31, 2023 09:44
@lsipii lsipii requested a review from LauriGofore October 31, 2023 14:59
_logger = logger;
}

public async Task Execute(string? _)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onko tällä anomuumilla string? parametrilla jokin funktio tässä?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Se toteuttaa tuon määrityksen IAdminAppAction vastaavasti kuin muissa "Action"-toteutuksissa, muualla sitä käytetään inputin parsimiseen raa-asta json-tekstistä. Voisihan siihen kyllä laittaa overriden että voisi kutsua myös ilman parametriä.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eikun eipä se yliajaminen tässä taida onnistuakaan ilman että lähtee muuttamaan rakannetta.

Comment on lines +77 to +82
var rawQuery = @"
SELECT COUNT(*) AS Amount, regexp_split_to_table(""Audiences"", ',') AS Audience
FROM ""ExternalIdentities""
GROUP BY Audience;
";
var personsCountByAudiences = _dataContext.PersonsByAudiencesResults.FromSqlRaw(rawQuery).ToList();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ilmeisesti raakaquery käytössä, koska LINQ ei tue regexiä?

Kysyin huvikseen kaikkitietävältä AI:lta mikä tälle ois vastine, en tiedä toimisiko ja onko edes yhtään selkeämpi:

var personsCountByAudiences = _dataContext.ExternalIdentities .SelectMany(e => e.Audiences.Split(','), (identity, audience) => new { Identity = identity, Audience = audience }) .GroupBy(x => x.Audience) .Select(g => new PersonsByAudiencesResult { Amount = g.Count(), Audience = g.Key }) .ToList();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hienolta näyttää löydys, täytyy kokeilla! Itellä oli hankaluuksia saada linqillä haluttuun muotoon ja tulokseen, muuten kuin päivittämällä ef core v6 -> v7 (ja siitä tulisi sitten muita ongelmia). Tässä "Audiences"-kolumnissa oli alkuperäinen tarkoitus tallentaa se tyypiksi text[], mutta ei oikein näyttänyt istuvan ef:n maisemaan. Luulen että samasta syystä projun muutkin teksti-listat on tallennettu kantaan comma-separated-tekstitietueena eikä ihan vaan listana. Työkalut muokkaa valintoja.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ei toimi tuollaisenaan esimerkki koska tuo on tosiaan entity frameworkin käpistelyssä listatyyppi ja sillä ei ole stringin split-metodia. Täytyy vielä tsekkailla josko tuon saisi kuitenkin text[]-tyypiksi oikeastikin, ilman että tarvitsee päivittää efcore versiota, tuli sen kanssa vain hieman jo käytettyä niitä kuuluisia pitkiä tunteja.


namespace VirtualFinland.UserAPI.Middleware;

public class AnalyticsMiddleware
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oiskohan tässä kenties joku selventävä kommentti, että tämä middleware hoitaa kaksi eri asiaa:

  • tracen passaaminen eteenpäin
  • analytiikan kerääminen

Nuo ei ainakaan omassa päässä käänny siten, että liittyisivät suoraan toisiinsa (toinen enemmän audit trail/log hommia[?]), mutta sikäli ihan ok hoitaa samassa paikassa (olikin tuosta näköjään poisteltu joku RequestTracinMiddleware).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Joo totta ne on omia vastuita ja hyvin voisi elellä omissa kalikoissaankin.

Copy link
Copy Markdown
Contributor

@LauriGofore LauriGofore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 nähty myös dashboard käppyröissä demoilun yhteydessä, hyvin kerääpi dadaa!

@lsipii lsipii merged commit 1e37cb6 into main Nov 13, 2023
@lsipii lsipii deleted the feat/analytics-update-events branch November 13, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants