res_pjsip/contacts/statsd: Make contact lifecycle events more consistent

It will never be perfect or even pretty, mostly because of the differences
between static and dynamic contacts.

Created:

Can't use the contact or contact_status alloc functions
because the objects come and go regardless of the actual state.

Can't use the contact_apply_handler, ast_sip_location_add_contact or
a sorcery created handler because they only get called for dynamic
contacts.  Similarly, permanent_uri_handler only gets called for
static contacts.

So, Matt had it right. :)  ast_res_pjsip_find_or_create_contact_status is
the only place it can go and not have duplicated code.  Both
permanent_uri_handler and contact_apply_handler call find_or_create.

Removed:

Can't use the destructors for the same reason as above.  The only
place to put this is in persistent_endpoint_contact_deleted_observer
which I believe is the "correct" place but even that will handle only
dynamic contacts.  This doesn't called on shutdown however.  There is
no hook to use for static contacts that may be removed because of a
config change while asterisk is in operation.

I moved the cleanup of contact_status from ast_sip_location_delete_contact
to the handler as well.

Status Change and RTT:

Although they worked fine where they were (in update_contact_status) I
moved them to persistent_endpoint_contact_status_observer to make it
more consistent with removed.  There was logic there already to detect
a state change.

Finally, fixed a nit in permanent_uri_handler rmudgett reported
eralier.

ASTERISK-25608 #close

Change-Id: I4b56e7dfc3be3baaaf6f1eac5b2068a0b79e357d
Reported-by: George Joseph
Tested-by: George Joseph

diff --git a/res/res_pjsip/location.c b/res/res_pjsip/location.c
index 170ada8512d0ce92619703c3720d137bb3055ad7..2908f6f70c85539607f94294e780027fb40af0cc 100644 (file)
--- a/res/res_pjsip/location.c
+++ b/res/res_pjsip/location.c
@@ -322,14 +322,6 @@ int ast_sip_location_update_contact(struct ast_sip_contact *contact)
 
 int ast_sip_location_delete_contact(struct ast_sip_contact *contact)
 {
-       void *contact_status_obj;
-
-       contact_status_obj = ast_sorcery_retrieve_by_id(ast_sip_get_sorcery(), CONTACT_STATUS, ast_sorcery_object_get_id(contact));
-       if (contact_status_obj) {
-               ast_sorcery_delete(ast_sip_get_sorcery(), contact_status_obj);
-               ao2_ref(contact_status_obj, -1);
-       }
-
        return ast_sorcery_delete(ast_sip_get_sorcery(), contact);
 }
 
@@ -394,7 +386,7 @@ static int permanent_uri_handler(const struct aco_option *opt, struct ast_variab
                struct ast_sip_contact *contact;
                struct ast_sip_contact_status *status;
                char hash[33];
-               char contact_id[strlen(aor_id) + sizeof(hash) + 2 + 1];
+               char contact_id[strlen(aor_id) + sizeof(hash) + 2];
 
                if (!aor->permanent_contacts) {
                        aor->permanent_contacts = ao2_container_alloc_list(AO2_ALLOC_OPT_LOCK_NOLOCK,
@@ -411,6 +403,8 @@ static int permanent_uri_handler(const struct aco_option *opt, struct ast_variab
                        return -1;
                }
 
+               ast_string_field_set(contact, uri, contact_uri);
+
                status = ast_res_pjsip_find_or_create_contact_status(contact);
                if (!status) {
                        ao2_ref(contact, -1);
@@ -418,7 +412,6 @@ static int permanent_uri_handler(const struct aco_option *opt, struct ast_variab
                }
                ao2_ref(status, -1);
 
-               ast_string_field_set(contact, uri, contact_uri);
                ao2_link(aor->permanent_contacts, contact);
                ao2_ref(contact, -1);
        }
@@ -1036,7 +1029,7 @@ int ast_sip_initialize_sorcery_location(void)
         * Note that this must done here, as contacts will create the contact_status
         * object before PJSIP options handling is initialized.
         */
-       for (i = 0; i < REMOVED; i++) {
+       for (i = 0; i <= REMOVED; i++) {
                ast_statsd_log_full_va("PJSIP.contacts.states.%s", AST_STATSD_GAUGE, 0, 1.0, ast_sip_get_contact_status_label(i));
        }
 

diff --git a/res/res_pjsip/pjsip_configuration.c b/res/res_pjsip/pjsip_configuration.c
index 04002ef9486e175c228d13aa92a6c5942c1f269f..1b0d6e2e94e366dc4c4b24b6a9d790c4dd65750d 100644 (file)
--- a/res/res_pjsip/pjsip_configuration.c
+++ b/res/res_pjsip/pjsip_configuration.c
@@ -20,6 +20,7 @@
 #include "asterisk/sorcery.h"
 #include "asterisk/callerid.h"
 #include "asterisk/test.h"
+#include "asterisk/statsd.h"
 
 /*! \brief Number of buckets for persistent endpoint information */
 #define PERSISTENT_BUCKETS 53
@@ -164,23 +165,21 @@ static void persistent_endpoint_contact_deleted_observer(const void *object)
        const struct ast_sip_contact *contact = object;
        struct ast_sip_contact_status *contact_status;
 
-       contact_status = ast_sorcery_alloc(ast_sip_get_sorcery(), CONTACT_STATUS,
-               ast_sorcery_object_get_id(contact));
+       contact_status = ast_sorcery_retrieve_by_id(ast_sip_get_sorcery(), CONTACT_STATUS, ast_sorcery_object_get_id(contact));
        if (!contact_status) {
                ast_log(LOG_ERROR, "Unable to create ast_sip_contact_status for contact %s/%s\n",
                        contact->aor, contact->uri);
                return;
        }
-       contact_status->uri = ast_strdup(contact->uri);
-       if (!contact_status->uri) {
-               ao2_cleanup(contact_status);
-               return;
-       }
-       contact_status->status = REMOVED;
 
        ast_verb(1, "Contact %s/%s has been deleted\n", contact->aor, contact->uri);
+       ast_statsd_log_string_va("PJSIP.contacts.states.%s", AST_STATSD_GAUGE,
+               "-1", 1.0, ast_sip_get_contact_status_label(contact_status->status));
+       ast_statsd_log_string_va("PJSIP.contacts.states.%s", AST_STATSD_GAUGE,
+               "+1", 1.0, ast_sip_get_contact_status_label(REMOVED));
 
        ao2_callback(persistent_endpoints, OBJ_NODATA, persistent_endpoint_update_state, contact_status);
+       ast_sorcery_delete(ast_sip_get_sorcery(), contact_status);
        ao2_cleanup(contact_status);
 }
 
@@ -200,24 +199,31 @@ static void persistent_endpoint_contact_status_observer(const void *object)
                return;
        }
 
-       if (contact_status->status == contact_status->last_status) {
-               ast_debug(3, "Contact %s/%s status didn't change: %s, RTT: %.3f msec\n",
-                       contact_status->aor, contact_status->uri, ast_sip_get_contact_status_label(contact_status->status),
-                       contact_status->rtt / 1000.0);
-               return;
-       } else {
+       if (contact_status->status != contact_status->last_status) {
                ast_verb(1, "Contact %s/%s is now %s.  RTT: %.3f msec\n", contact_status->aor, contact_status->uri,
                        ast_sip_get_contact_status_label(contact_status->status),
                        contact_status->rtt / 1000.0);
-       }
 
-       ast_test_suite_event_notify("AOR_CONTACT_UPDATE",
-               "Contact: %s\r\n"
-                       "Status: %s",
-               ast_sorcery_object_get_id(contact_status),
-               ast_sip_get_contact_status_label(contact_status->status));
+               ast_statsd_log_string_va("PJSIP.contacts.states.%s", AST_STATSD_GAUGE,
+                       "-1", 1.0, ast_sip_get_contact_status_label(contact_status->last_status));
+               ast_statsd_log_string_va("PJSIP.contacts.states.%s", AST_STATSD_GAUGE,
+                       "+1", 1.0, ast_sip_get_contact_status_label(contact_status->status));
 
-       ao2_callback(persistent_endpoints, OBJ_NODATA, persistent_endpoint_update_state, contact_status);
+               ast_test_suite_event_notify("AOR_CONTACT_UPDATE",
+                       "Contact: %s\r\n"
+                               "Status: %s",
+                       ast_sorcery_object_get_id(contact_status),
+                       ast_sip_get_contact_status_label(contact_status->status));
+
+               ao2_callback(persistent_endpoints, OBJ_NODATA, persistent_endpoint_update_state, contact_status);
+       } else {
+               ast_debug(3, "Contact %s/%s status didn't change: %s, RTT: %.3f msec\n",
+                       contact_status->aor, contact_status->uri, ast_sip_get_contact_status_label(contact_status->status),
+                       contact_status->rtt / 1000.0);
+       }
+
+       ast_statsd_log_full_va("PJSIP.contacts.%s.rtt", AST_STATSD_TIMER,
+               contact_status->status != AVAILABLE ? -1 : contact_status->rtt / 1000, 1.0, ast_sorcery_object_get_id(contact_status));
 }
 
 /*! \brief Observer for contacts so state can be updated on respective endpoints */

diff --git a/res/res_pjsip/pjsip_options.c b/res/res_pjsip/pjsip_options.c
index e0557871ed7bbc449ea5b7656df24ae0578c1f3d..8b75a5ff44fbd5d009423959829e812d8d476b9d 100644 (file)
--- a/res/res_pjsip/pjsip_options.c
+++ b/res/res_pjsip/pjsip_options.c
@@ -181,22 +181,13 @@ static void update_contact_status(const struct ast_sip_contact *contact,
 
        update->last_status = status->status;
        update->status = value;
-       if (update->last_status != update->status) {
-               ast_statsd_log_string_va("PJSIP.contacts.states.%s", AST_STATSD_GAUGE,
-                       "-1", 1.0, ast_sip_get_contact_status_label(update->last_status));
-               ast_statsd_log_string_va("PJSIP.contacts.states.%s", AST_STATSD_GAUGE,
-                       "+1", 1.0, ast_sip_get_contact_status_label(update->status));
-       }
 
        /* if the contact is available calculate the rtt as
           the diff between the last start time and "now" */
        update->rtt = update->status == AVAILABLE && status->rtt_start.tv_sec > 0 ?
                ast_tvdiff_us(ast_tvnow(), status->rtt_start) : 0;
-
        update->rtt_start = ast_tv(0, 0);
 
-       ast_statsd_log_full_va("PJSIP.contacts.%s.rtt", AST_STATSD_TIMER,
-               update->rtt / 1000, 1.0, ast_sorcery_object_get_id(update));
        ast_test_suite_event_notify("AOR_CONTACT_QUALIFY_RESULT",
                "Contact: %s\r\n"
                "Status: %s\r\n"