Turn Alerts Proof of concept #203
Conversation
config/settings/base.py
Outdated
|
|
||
| # Turn Alerts | ||
|
|
||
| TURN_ALERTS_JOURNEY_URL = env.str("TURN_ALERTS_JOURNEY_URL", None) |
There was a problem hiding this comment.
We can maybe use the details that we configure per org: https://github.com/praekeltfoundation/rp-sidekick/blob/develop/sidekick/models.py#L21-L22
I think check if they are still being used elsewhere then we take it from there.
The idea with sidekick was always to allow multi-tenancy, for MC both QA and prod run in one instance with different orgs configured. This means you need the org id in the url like the interceptor.
turn_alerts/models.py
Outdated
| from django.db import models | ||
|
|
||
|
|
||
| class TurnAlerts(models.Model): |
There was a problem hiding this comment.
Whats the reason for saving these? I think we want to avoid that if possible
turn_alerts/tasks.py
Outdated
| data = {"wa_id": wa_id} | ||
|
|
||
| response = requests.post( | ||
| settings.TURN_ALERTS_JOURNEY_URL, headers=headers, json=data |
There was a problem hiding this comment.
If we use the url from the org we just need to add the correct path on to it here.
turn_alerts/views.py
Outdated
| request_count = Counter( | ||
| "turn_alerts_request_total", "Total number of requests to TurnAlertsLayerView" | ||
| ) | ||
| processing_time = Histogram( | ||
| "turn_alerts_request_processing_seconds", "Processing time of requests in seconds" | ||
| ) |
There was a problem hiding this comment.
I think we're more interested in how many messages and events are coming through.
Messages grouped by fallback, direction, type
Events grouped by status, code etc
I think in the gist we group events by type as an example
turn_alerts/views.py
Outdated
| "fallback_channel", | ||
| "direction", | ||
| "message_type", | ||
| "message_body", |
There was a problem hiding this comment.
I'm not sure if we need all these, first 3 is probably fine
turn_alerts/views.py
Outdated
| event_count = Counter( | ||
| "turn_alerts_event_count", | ||
| "Number of events", | ||
| ["message_status", "error_code", "error_data"], | ||
| ) | ||
|
|
||
| successful_event_count = Counter( | ||
| "turn_alerts_successful_event_count", | ||
| "Number of events", | ||
| ["message_type", "message_status", "conversation_id"], | ||
| ) |
There was a problem hiding this comment.
Do we need two?
Also not sure if error_data would be useful in grafana
turn_alerts/views.py
Outdated
| ) | ||
|
|
||
|
|
||
| processing_time = Histogram( |
There was a problem hiding this comment.
Funtionally no but I added this just to see how the buckets thing works to split the proessing times. I can remove.
turn_alerts/models.py
Outdated
| from sidekick.models import Organization | ||
|
|
||
|
|
||
| class TurnAlerts(models.Model): |
There was a problem hiding this comment.
What's the reason for saving this?
There was a problem hiding this comment.
The reason I have this saved is so it can be configurable. Do we want to hardcode the journey_id etc or have it easily changed.
There was a problem hiding this comment.
Also, we no longer need at least half of the serializers there since we no longer use most of the key values for prometheus. I left them there just in case we needed to capture more data. I could delete most of them if you think otherwise. @erikh360
There was a problem hiding this comment.
Yes please remove all serializers we don't need.
Sorry I misunderstood this model. Lets chat in Slack
turn_alerts/tasks.py
Outdated
| organization = turn_alerts.org | ||
|
|
||
| headers = { | ||
| "Authorization": f"Bearer {turn_alerts.hmac_secret}", |
There was a problem hiding this comment.
The HMAC secret is something we use to validate the the request came from Turn, since the endpoint doesn't have auth on.
This should be the engage token from the org model
turn_alerts/tasks.py
Outdated
| ) | ||
| def start_turn_journey(wa_id): | ||
|
|
||
| turn_alerts = TurnAlerts.objects.get(pk=1) |
turn_alerts/views.py
Outdated
| conversation_id=conversation_id, | ||
| conversation_type=conversation_type, |
There was a problem hiding this comment.
It's part of the status payload from Turn for the id and type of conversation. An example of the conversation type I see is "service". I left the ID too just in case there's an event and we need to track the conversation. Not sure what other use case would need it. @erikh360
turn_alerts/utils.py
Outdated
| from sidekick.models import Organization | ||
|
|
||
|
|
||
| class TurnAlerts(models.Model): |
There was a problem hiding this comment.
I see we have it defined in models.py and here?
There was a problem hiding this comment.
It's for testing. It creates an instance of turnalerts with the configurable objects for the org. Ex, error_code.
turn_alerts/views.py
Outdated
| TurnOutboundSerializer(data=request.data).is_valid(raise_exception=True) | ||
| outbound = request.data | ||
| direction = outbound["_vnd"]["v1"].get("direction") | ||
| message_type = (outbound.get("type", ""),) |
turn_alerts/views.py
Outdated
| elif webhook_type == "turn": | ||
| TurnOutboundSerializer(data=request.data).is_valid(raise_exception=True) | ||
| outbound = request.data | ||
| direction = outbound["_vnd"]["v1"].get("direction") |
There was a problem hiding this comment.
Is this sometimes not available? I would assume its always there, message can only be inbound or outbound
There was a problem hiding this comment.
I haven't seen it not available but I just used a get in case it happens albeit quite unlikely.
turn_alerts/views.py
Outdated
| ) | ||
|
|
||
| else: | ||
| recipient_id = statuses.get("recipient_id") |
There was a problem hiding this comment.
No, only need for the falied event that starts the journey.
turn_alerts/views.py
Outdated
| event_count = Counter( | ||
| "turn_alerts_event_count", | ||
| "Number of events", | ||
| ["message_status", "error_code", "conversation_id", "conversation_type"], |
There was a problem hiding this comment.
I don't think we'll ever aggregate on conversation_id?
What are the possible values for conversation_type?
There was a problem hiding this comment.
Is it possible to add fallback channel here?
There was a problem hiding this comment.
The only conversation type I've seen so far is "service" and I'm not sure what it means. We can have fallbackchannel. I'll add that.
turn_alerts/views.py
Outdated
| on_fallback_channel = request.headers.get("X-Turn-Fallback-Channel", "0") == "1" | ||
| is_turn_event = request.headers.get("X-Turn-Event", "0") == "1" | ||
|
|
||
| for alert in turn_alerts: |
There was a problem hiding this comment.
How will this work if we have multiple alerts configured?
There was a problem hiding this comment.
I'm not clear on the question. I ahve at least two alerts configured locally for two separate orgs that I've been testing with.
There was a problem hiding this comment.
Yes here you are filtering by organisation, so 1 per org will only return 1.
If you have 2 on one org, journey_id, org_error_code, engage_token and engage_url gets overwritten for each alert found.
I would rather add the error code and journey id to a dict, which you can then check in the loop where we get the error code. The engage_* doesn't have to be set in the alert loop since we're only dealing with one org at a time.
turn_alerts/views.py
Outdated
| match = re.match( | ||
| ( | ||
| r"^\s*(?:\+?(\d{1,3}))?[-. (]*(\d{3})[-. )]" | ||
| r"*(\d{3})[-. ]*(\d{4})(?: *x(\d+))?\s*$" | ||
| ), | ||
| recipient_id, | ||
| ) |
There was a problem hiding this comment.
Checks that the recipient id matches a valid SA number. If yes, start the journey task otherwise don't.
There was a problem hiding this comment.
Do we filter on SA phone numbers anywhere else?
…tional tests and updates
turn_alerts/views.py
Outdated
| on_fallback_channel = request.headers.get("X-Turn-Fallback-Channel", "0") == "1" | ||
| is_turn_event = request.headers.get("X-Turn-Event", "0") == "1" | ||
|
|
||
| for alert in turn_alerts: |
There was a problem hiding this comment.
Yes here you are filtering by organisation, so 1 per org will only return 1.
If you have 2 on one org, journey_id, org_error_code, engage_token and engage_url gets overwritten for each alert found.
I would rather add the error code and journey id to a dict, which you can then check in the loop where we get the error code. The engage_* doesn't have to be set in the alert loop since we're only dealing with one org at a time.
turn_alerts/views.py
Outdated
| match = re.match( | ||
| ( | ||
| r"^\s*(?:\+?(\d{1,3}))?[-. (]*(\d{3})[-. )]" | ||
| r"*(\d{3})[-. ]*(\d{4})(?: *x(\d+))?\s*$" | ||
| ), | ||
| recipient_id, | ||
| ) |
There was a problem hiding this comment.
Do we filter on SA phone numbers anywhere else?
turn_alerts/views.py
Outdated
| "conversation_id", | ||
| "conversation_type", |
turn_alerts/tests/utils.py
Outdated
| from turn_alerts.models import TurnActions | ||
|
|
||
|
|
||
| def create_turnalerts_account(org=None, **kwargs): |
There was a problem hiding this comment.
This function is called create_turnalerts_account but is not creating an account?
turn_alerts/tests/utils.py
Outdated
| from turn_alerts.models import TurnActions | ||
|
|
||
|
|
||
| def create_turnalerts(org=None, **kwargs): |
There was a problem hiding this comment.
This function name is still confusing
turn_alerts/tests/test_views.py
Outdated
| self.client.force_authenticate(user) | ||
|
|
||
| self.org = create_org() | ||
| self.turnalerts_account = create_turnalerts(org=self.org) |
There was a problem hiding this comment.
create_org() creates the org object that is being used here:
url = reverse("turn-messages", kwargs={'org_id' : self.org.id})
turn_alerts/tests/test_views.py
Outdated
| self.client.force_authenticate(user) | ||
|
|
||
| self.org = create_org() | ||
| self.turnalerts_account = create_turnalerts(org=self.org) |
turn_alerts/tests/test_views.py
Outdated
|
|
||
| url = reverse("turn-messages", kwargs={"org_id": self.org.id}) | ||
|
|
||
| with patch("requests.post") as mock_post: |
There was a problem hiding this comment.
Can we use responses to mock requests? or is this how we do it in the reset of sidekick?
turn_alerts/tests/test_views.py
Outdated
| with patch.object(TurnActions.objects, "filter", return_value=[turn_action]): | ||
| with patch.object(Organization.objects, "get", return_value=self.org): |
There was a problem hiding this comment.
Can we not just create the correct objects here? The org definitely exists so that patch doesn't look like we need it
turn_alerts/tests/test_views.py
Outdated
| view = TurnAlertsLayerView() | ||
| response = view.post(request, org_id=1) |
There was a problem hiding this comment.
Any reason for doing it this way and not using the client created in the setUp() function?
config/settings/base.py
Outdated
| @@ -226,3 +227,5 @@ | |||
| ) | |||
|
|
|||
| REDIS_URL = os.environ.get("REDIS_URL", "redis://localhost:6379/0") | |||
|
|
|||
| TURN_HMAC_SECRET = os.environ.get("TURN_HMAC_SECRET", "REPLACEME") | |||
There was a problem hiding this comment.
This is specific to each webhook configured in turn
…urnActions. Also add secret_id to url
turn_alerts/models.py
Outdated
| class TurnSecret(models.Model): | ||
| org = models.ForeignKey(Organization, on_delete=models.CASCADE) | ||
| secret = models.CharField(max_length=255) | ||
| help_text = "The secret for validating webhook requests" |
There was a problem hiding this comment.
Not sure if this help text should be here?
turn_alerts/utils.py
Outdated
|
|
||
| def validate_signature(request, org_id, turn_secret_id): | ||
| try: | ||
| organization = Organization.objects.get(id=org_id) |
There was a problem hiding this comment.
This will mean we're doing the org lookup twice, remove this and replace the line under it:
turn_secret = TurnSecret.objects.get(id=turn_secret_id, org_id=org_id)
and remove the org exception
Purpose
We need a way to alert when turn messages are failing to send and deliver.
We also need a way to act on these events for specific contacts, for example a specific error code starts the switch to SMS.
Solution
An app that receives Turn messages and exposes event metrics to prometheus.
Checklist