Skip to content
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

[#IC-87] Dismantle durable functions #169

Merged
merged 2 commits into from
Nov 22, 2021
Merged

Conversation

balanza
Copy link
Contributor

@balanza balanza commented Nov 14, 2021

Motivation and Context

Following #168 we remove unused code

List of Changes

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Chore (nothing changes by a user perspective)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@pagopa-github-bot
Copy link
Contributor

pagopa-github-bot commented Nov 14, 2021

Warnings
⚠️ This PR changes a total of 3566 LOCs, that is more than a reasonable size of 250. Consider splitting the pull request into smaller ones.
⚠️ Please include a Pivotal story at the beginning of the PR title (see below).

Example of PR titles that include pivotal stories:

  • single story: [#123456] my PR title
  • multiple stories: [#123456,#123457,#123458] my PR title

Generated by 🚫 dangerJS against 53cbceb

@balanza balanza marked this pull request as ready for review November 19, 2021 10:59
@balanza balanza requested a review from a team as a code owner November 19, 2021 10:59
@balanza balanza force-pushed the ic-87--dismantle-orchestrator branch from fda8cb6 to 83c70fb Compare November 19, 2021 11:25
Copy link
Contributor

@BurnedMarshal BurnedMarshal left a comment

Choose a reason for hiding this comment

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

LGTM

@balanza balanza merged commit 21abf11 into master Nov 22, 2021
@balanza balanza deleted the ic-87--dismantle-orchestrator branch November 22, 2021 08:36
@balanza balanza mentioned this pull request Nov 22, 2021
6 tasks
@gunzip
Copy link
Contributor

gunzip commented Nov 29, 2021

For [REDACTED] project we need to collect several metrics that can tell us how much time is elapsed between a message reaches the io-function-services HTTP controller and:

  1. the moment it becomes visible in the user inbox (I guess, when it's stored into cosmosdb + storage account and the status switches to "processed")
  2. the moment the webhook is called

Ideally: the time spent in each step of the message path.

This need is due to the reason we probably need some priority mechanism to deliver the messages that come from some "critical" services, but currently we don't know the average time needed to deliver one message (inbox + push, email does not matter atm). If the average time is under, lets say 10 seconds, even under peak loads, probably we won't need a refactor at all.

Merely this rector cuts out some formerly tracked events that could have helped here.

I've read the IO-RFC with the analysis of probable bottlenecks and, as far as I understood, the main ones are storage queues (Am I missing something @BurnedMarshal ?).

Another option, to get some insights, could be to monitor the length of the queues which are used currently.

What do you think about that?

@BurnedMarshal @balanza

@balanza
Copy link
Contributor Author

balanza commented Nov 29, 2021

Merely this rector cuts out some formerly tracked events

We can reintroduce such events with little effort. Please keep in mind that such events are (and were) sampled, so that you can easily quantify the % of processed/failing/whatever messages, but you cannot trace a single message through the entire flow.

What we might want to achieve is a transactional tracing of some form, so that we can follow (every? some?) messages in each step from the sender to the Citizen's inbox.

@gunzip
Copy link
Contributor

gunzip commented Nov 29, 2021

keep in mind that such events are (and were) sampled

No they werent' as a specific flag was introduced by @infantesimone to avoid sampling those events.

Anyway, it's ok even if they're sampled since what we want here is the average timing.

The query on application insights would group by (messageId).

@pasqualedevita currently we have two custom AI events:

  • api.messages.create
  • api.messages.notification.push.sent

do you think it's possibile to get the average difference time(api.messages.create) - time(api.messages.notification.push.sent) querying AI (grouping by messageid) ?

@balanza
Copy link
Contributor Author

balanza commented Nov 29, 2021

No they werent' as a specific flag was introduced by @infantesimone to avoid sampling those events.

AFAIK we had empiric evidence that sampling was applied anyway, but I may be wrong.

@pasqualedevita
Copy link
Member

pasqualedevita commented Nov 29, 2021

do you think it's possibile to get the average difference time(api.messages.create) - time(api.messages.notification.push.sent) querying AI (grouping by messageid) ?

I think is possibile (not simple) but using app insight can be not accurate due to sampling and we don't know if u can disable sampling with custom events.
Also using app insights u are locked in Azure solution.

As alternative I want to propose to send relevant events in a kafka queue (eventhub). In this way u can read the queue and create all kpi that u need in PDND dashboards (filer by sender service, priority, average, percentile distribution, ecc)

PS: I think we are under 10s, probably now the bottleneck is only on fn-pushnotification

@gunzip
Copy link
Contributor

gunzip commented Nov 29, 2021

If you group by messageId sampling shouldn't matter since any of those two values would be simply undefined (you have to filter out undefined entries).

I think we are under 10s, probably now the bottleneck is only on fn-pushnotification

great!

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.

5 participants