Skip to content

Commit

Permalink
refactor(axiom): decoupled cloud attributes from message segment
Browse files Browse the repository at this point in the history
Separated cloud attributes from message segment.
They can be applied to multiple segment types and don't belong in typed attributes.
It will make it easier to add to other segments in the future.

Added a function to add the cloud attributes.

Reworked unit tests with new functionality.
  • Loading branch information
zsistla committed Dec 28, 2024
1 parent 8461d6c commit b342fc5
Show file tree
Hide file tree
Showing 16 changed files with 256 additions and 287 deletions.
23 changes: 15 additions & 8 deletions agent/lib_aws_sdk_php.c
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ void nr_lib_aws_sdk_php_sqs_handle(nr_segment_t* segment, NR_EXECUTE_PROTO) {
.destination_type = NR_MESSAGE_DESTINATION_TYPE_QUEUE,
.messaging_system = "aws_sqs",
};
nr_segment_cloud_attrs_t cloud_attrs = {0};

if (NULL == segment) {
return;
Expand All @@ -131,7 +132,7 @@ void nr_lib_aws_sdk_php_sqs_handle(nr_segment_t* segment, NR_EXECUTE_PROTO) {
}

if (instrumented) {
message_params.aws_operation = command_name_string;
cloud_attrs.aws_operation = command_name_string;

command_arg_value = nr_lib_aws_sdk_php_get_command_arg_value(
AWS_SDK_PHP_SQSCLIENT_QUEUEURL_ARG, NR_EXECUTE_ORIG_ARGS);
Expand All @@ -140,7 +141,12 @@ void nr_lib_aws_sdk_php_sqs_handle(nr_segment_t* segment, NR_EXECUTE_PROTO) {
* nr_lib_aws_sdk_php_sqs_parse_queueurl checks for NULL so safe pass
* command_arg_value directly in.
*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(command_arg_value, &message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(command_arg_value, &message_params,
&cloud_attrs);

/* Add cloud attributes, if available. */

nr_segment_traces_add_cloud_attributes(segment, &cloud_attrs);

/* Now end the instrumented segment as a message segment. */
nr_segment_message_end(&segment, &message_params);
Expand All @@ -153,14 +159,15 @@ void nr_lib_aws_sdk_php_sqs_handle(nr_segment_t* segment, NR_EXECUTE_PROTO) {
* These are the message_params params that were strduped in
* nr_lib_aws_sdk_php_sqs_parse_queueurl
*/
nr_free(message_params.cloud_region);
nr_free(cloud_attrs.cloud_region);
nr_free(message_params.destination_name);
nr_free(message_params.cloud_account_id);
nr_free(cloud_attrs.cloud_account_id);
}

void nr_lib_aws_sdk_php_sqs_parse_queueurl(
const char* sqs_queueurl,
nr_segment_message_params_t* message_params) {
nr_segment_message_params_t* message_params,
nr_segment_cloud_attrs_t* cloud_attrs) {
char* region = NULL;
char* queue_name = NULL;
char* account_id = NULL;
Expand All @@ -184,7 +191,7 @@ void nr_lib_aws_sdk_php_sqs_parse_queueurl(
*
* Due to the overhead involved in escaping the original buffer, creating a
* regex, matching a regex, destroying a regex, this method was chosen as a
* more performant option.
* more performant option because it's a very limited pattern.
*/

if (NULL == nr_strlcpy(queueurl, sqs_queueurl, AWS_QUEUEURL_LEN_MAX)) {
Expand Down Expand Up @@ -293,8 +300,8 @@ void nr_lib_aws_sdk_php_sqs_parse_queueurl(
* cloud.account.id, messaging.destination.name
*/
message_params->destination_name = nr_strdup(queue_name);
message_params->cloud_account_id = nr_strdup(account_id);
message_params->cloud_region = nr_strdup(region);
cloud_attrs->cloud_account_id = nr_strdup(account_id);
cloud_attrs->cloud_region = nr_strdup(region);
}

char* nr_lib_aws_sdk_php_get_command_arg_value(char* command_arg_name,
Expand Down
13 changes: 8 additions & 5 deletions agent/lib_aws_sdk_php.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,20 @@ extern void nr_lib_aws_sdk_php_add_supportability_service_metric(
* same string and if it is malformed, it cannot be used.
*
* Params : 1. The QueueUrl
* 2. message_params: the extracted values will be set to
* message_params.cloud_region, message_params.cloud_account_id, and
* message_params.destination_name Returns :
* 2. message_params to set message_params.destination_name
* 3. cloud_attrs to set message_params.cloud_region,
* message_params.cloud_account_id
*
* Returns :
*
* Note: caller is responsible for
* freeing message_params.cloud_region, message_params.cloud_account_id, and
* freeing cloud_attrs.cloud_region, cloud_attrs.cloud_account_id, and
* message_params.destination_name
*/
extern void nr_lib_aws_sdk_php_sqs_parse_queueurl(
const char* sqs_queueurl,
nr_segment_message_params_t* message_params);
nr_segment_message_params_t* message_params,
nr_segment_cloud_attrs_t* cloud_attrs);

/*
* Purpose : Handle when an SqsClient initiates a command
Expand Down
72 changes: 42 additions & 30 deletions agent/tests/test_lib_aws_sdk_php.c
Original file line number Diff line number Diff line change
Expand Up @@ -265,27 +265,28 @@ static void test_nr_lib_aws_sdk_php_get_command_name() {

static inline void test_message_param_queueurl_settings_expect_val(
nr_segment_message_params_t message_params,
nr_segment_cloud_attrs_t cloud_attrs,
char* cloud_region,
char* cloud_account_id,
char* destination_name) {
tlib_pass_if_str_equal("cloud_region should match.",
message_params.cloud_region, cloud_region);
tlib_pass_if_str_equal("cloud_region should match.", cloud_attrs.cloud_region,
cloud_region);
tlib_pass_if_str_equal("cloud_account_id should match.",
message_params.cloud_account_id, cloud_account_id);
cloud_attrs.cloud_account_id, cloud_account_id);
tlib_pass_if_str_equal("destination_name should match.",
message_params.destination_name, destination_name);

nr_free(message_params.cloud_region);
nr_free(message_params.cloud_account_id);
nr_free(cloud_attrs.cloud_region);
nr_free(cloud_attrs.cloud_account_id);
nr_free(message_params.destination_name);
}

static inline void test_message_param_queueurl_settings_expect_null(
nr_segment_message_params_t message_params) {
tlib_pass_if_null("cloud_region should be null.",
message_params.cloud_region);
nr_segment_message_params_t message_params,
nr_segment_cloud_attrs_t cloud_attrs) {
tlib_pass_if_null("cloud_region should be null.", cloud_attrs.cloud_region);
tlib_pass_if_null("cloud_account_id should be null.",
message_params.cloud_account_id);
cloud_attrs.cloud_account_id);
tlib_pass_if_null("destination_name should be null.",
message_params.destination_name);
}
Expand All @@ -296,6 +297,7 @@ static void test_nr_lib_aws_sdk_php_sqs_parse_queueurl() {
* cloud_account_id, and destination_name or none.
*/
nr_segment_message_params_t message_params = {0};
nr_segment_cloud_attrs_t cloud_attrs = {0};

tlib_php_engine_create("");

Expand All @@ -312,48 +314,58 @@ static void test_nr_lib_aws_sdk_php_sqs_parse_queueurl() {
// clang-format on

/* Test null queueurl. Extracted message_param values should be null.*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(NULL, &message_params);
test_message_param_queueurl_settings_expect_null(message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(NULL, &message_params, &cloud_attrs);
test_message_param_queueurl_settings_expect_null(message_params, cloud_attrs);

/* Test Invalid values. Extracted message_param values should be null.*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_1, &message_params);
test_message_param_queueurl_settings_expect_null(message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_1, &message_params,
&cloud_attrs);
test_message_param_queueurl_settings_expect_null(message_params, cloud_attrs);

/* Test Invalid values. Extracted message_param values should be null.*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_2, &message_params);
test_message_param_queueurl_settings_expect_null(message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_2, &message_params,
&cloud_attrs);
test_message_param_queueurl_settings_expect_null(message_params, cloud_attrs);

/* Test Invalid values. Extracted message_param values should be null.*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_3, &message_params);
test_message_param_queueurl_settings_expect_null(message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_3, &message_params,
&cloud_attrs);
test_message_param_queueurl_settings_expect_null(message_params, cloud_attrs);

/* Test Invalid values. Extracted message_param values should be null.*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_4, &message_params);
test_message_param_queueurl_settings_expect_null(message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_4, &message_params,
&cloud_attrs);
test_message_param_queueurl_settings_expect_null(message_params, cloud_attrs);

/* Test Invalid values. Extracted message_param values should be null.*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_5, &message_params);
test_message_param_queueurl_settings_expect_null(message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_5, &message_params,
&cloud_attrs);
test_message_param_queueurl_settings_expect_null(message_params, cloud_attrs);

/* Test Invalid values. Extracted message_param values should be null.*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_6, &message_params);
test_message_param_queueurl_settings_expect_null(message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_6, &message_params,
&cloud_attrs);
test_message_param_queueurl_settings_expect_null(message_params, cloud_attrs);

/* Test Invalid values. Extracted message_param values should be null.*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_7, &message_params);
test_message_param_queueurl_settings_expect_null(message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_7, &message_params,
&cloud_attrs);
test_message_param_queueurl_settings_expect_null(message_params, cloud_attrs);

/* Test Invalid values. Extracted message_param values should be null.*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_8, &message_params);
test_message_param_queueurl_settings_expect_null(message_params);
nr_lib_aws_sdk_php_sqs_parse_queueurl(INVALID_QUEUE_URL_8, &message_params,
&cloud_attrs);
test_message_param_queueurl_settings_expect_null(message_params, cloud_attrs);

/*
* Test 'https://sqs.us-east-2.amazonaws.com/123456789012/SQS_QUEUE_NAME'.
* Extracted message_param values should set.
*/
nr_lib_aws_sdk_php_sqs_parse_queueurl(VALID_QUEUE_URL, &message_params);
test_message_param_queueurl_settings_expect_val(
message_params, "us-east-2", "123456789012", "SQS_QUEUE_NAME");
nr_lib_aws_sdk_php_sqs_parse_queueurl(VALID_QUEUE_URL, &message_params,
&cloud_attrs);
test_message_param_queueurl_settings_expect_val(message_params, cloud_attrs,
"us-east-2", "123456789012",
"SQS_QUEUE_NAME");

tlib_php_engine_destroy();
}
Expand Down
14 changes: 0 additions & 14 deletions axiom/nr_segment.c
Original file line number Diff line number Diff line change
Expand Up @@ -329,18 +329,8 @@ static void nr_populate_message_spans(nr_span_event_t* span_event,
nr_span_event_set_message(
span_event, NR_SPAN_MESSAGE_MESSAGING_SYSTEM,
segment->typed_attributes->message.messaging_system);
nr_span_event_set_message(span_event, NR_SPAN_MESSAGE_CLOUD_REGION,
segment->typed_attributes->message.cloud_region);
nr_span_event_set_message(
span_event, NR_SPAN_MESSAGE_CLOUD_ACCOUNT_ID,
segment->typed_attributes->message.cloud_account_id);
nr_span_event_set_message(
span_event, NR_SPAN_MESSAGE_CLOUD_RESOURCE_ID,
segment->typed_attributes->message.cloud_resource_id);
nr_span_event_set_message(span_event, NR_SPAN_MESSAGE_SERVER_ADDRESS,
segment->typed_attributes->message.server_address);
nr_span_event_set_message(span_event, NR_SPAN_MESSAGE_AWS_OPERATION,
segment->typed_attributes->message.aws_operation);
}

static nr_status_t add_user_attribute_to_span_event(const char* key,
Expand Down Expand Up @@ -649,12 +639,8 @@ bool nr_segment_set_message(nr_segment_t* segment,
segment->typed_attributes->message = (nr_segment_message_t){
.message_action = message->message_action,
.destination_name = message->destination_name ? nr_strdup(message->destination_name) : NULL,
.cloud_region = message->cloud_region ? nr_strdup(message->cloud_region) : NULL,
.cloud_account_id = message->cloud_account_id ? nr_strdup(message->cloud_account_id) : NULL,
.messaging_system = message->messaging_system ? nr_strdup(message->messaging_system) : NULL,
.cloud_resource_id = message->cloud_resource_id ? nr_strdup(message->cloud_resource_id) : NULL,
.server_address = message->server_address ? nr_strdup(message->server_address) : NULL,
.aws_operation = message->aws_operation ? nr_strdup(message->aws_operation) : NULL,
};
// clang-format on

Expand Down
28 changes: 21 additions & 7 deletions axiom/nr_segment.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,21 +122,35 @@ typedef struct _nr_segment_message_t {
*/

nr_span_spankind_t
message_action; /*The action of the message, e.g.,Produce/Consume.*/
char* destination_name; /*The name of the Queue, Topic, or Exchange;
otherwise, Temp. Needed for SQS relationship.*/
message_action; /*The action of the message, e.g.,Produce/Consume.*/
char* destination_name; /*The name of the Queue, Topic, or Exchange;
otherwise, Temp. Needed for SQS relationship.*/
char* messaging_system; /* for ex: aws_sqs. Needed for SQS relationship.*/
char* server_address; /*The server domain name or IP address. Needed for
MQBROKER relationship.*/
} nr_segment_message_t;

typedef struct _nr_segment_cloud_attrs_t {
/*
* Attributes needed for entity relationship building.
* Compare to OTEL attributes:
* https://opentelemetry.io/docs/specs/semconv/attributes-registry/cloud/
* cloud.account.id, cloud.region, messaging.system and server.address are
* used to create relationships between APM and cloud services. It may not
* make sense to add these attributes unless they are used for creating one of
* the relationships in Entity Relationships.
* These attributes aren't specific to a segment category so don't belong as
* typed attributes and can be added whenever they are available.
*/
char* cloud_region; /*Targeted region; ex:us-east-1*. Needed for SQS
relationship.*/
char* cloud_account_id; /*The cloud provider account ID. Needed for SQS
relationship.*/
char* messaging_system; /* for ex: aws_sqs. Needed for SQS relationship.*/
char* cloud_resource_id; /*Unique cloud provider identifier. For AWS, this is
the ARN of the AWS resource being accessed.*/
char* server_address; /*The server domain name or IP address. Needed for
MQBROKER relationship.*/
char* aws_operation; /*AWS specific operation name.*/

} nr_segment_message_t;
} nr_segment_cloud_attrs_t;

typedef struct _nr_segment_metric_t {
char* name;
Expand Down
10 changes: 0 additions & 10 deletions axiom/nr_segment_message.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,11 @@ static void nr_segment_message_set_attrs(
message_attributes.destination_name = nr_strempty(params->destination_name)
? NULL
: params->destination_name;
message_attributes.cloud_region
= nr_strempty(params->cloud_region) ? NULL : params->cloud_region;
message_attributes.cloud_account_id = nr_strempty(params->cloud_account_id)
? NULL
: params->cloud_account_id;
message_attributes.messaging_system = nr_strempty(params->messaging_system)
? NULL
: params->messaging_system;
message_attributes.cloud_resource_id
= nr_strempty(params->cloud_resource_id) ? NULL
: params->cloud_resource_id;
message_attributes.server_address
= nr_strempty(params->server_address) ? NULL : params->server_address;
message_attributes.aws_operation
= nr_strempty(params->aws_operation) ? NULL : params->aws_operation;
}

nr_segment_set_message(segment, &message_attributes);
Expand Down
23 changes: 7 additions & 16 deletions axiom/nr_segment_message.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#define NR_SEGMENT_MESSAGE_HDR

#include "nr_segment.h"
#include "nr_segment_traces.h"

/*
* Note:
Expand All @@ -25,28 +26,18 @@ typedef struct {
/* All strings are null-terminated. When unset, the strings are ingored. */

/* Only used for creating metrics. */

char* library; /* Library; Possible values are SQS, SNS, RabbitMQ, JMS */
nr_segment_message_destination_type_t
destination_type; /* Named/temp queue/topic/exchange */

/* Used for creating message attributes. */
nr_span_spankind_t
message_action; /*The action of the message, e.g.,Produce/Consume.*/
char* destination_name; /*The name of the Queue, Topic, or Exchange;
otherwise, Temp. Needed for SQS relationship.*/
char* cloud_region; /*Targeted region; ex:us-east-1*. Needed for SQS
relationship.*/
char* cloud_account_id; /*The cloud provider account ID. Needed for SQS
relationship.*/
char* messaging_system; /* for ex: aws_sqs. Needed for SQS relationship.*/
char* cloud_resource_id; /*A unique identifier given by the cloud resource.
For AWS, this is the ARN of the AWS resource being
accessed.*/
char* server_address; /*The server domain name or IP address. Needed for
MQBROKER relationship.*/
char* aws_operation; /*The AWS operation being called.*/

message_action; /*The action of the message, e.g.,Produce/Consume.*/
char* destination_name; /*The name of the Queue, Topic, or Exchange;
otherwise, Temp. Needed for SQS relationship.*/
char* messaging_system; /* for ex: aws_sqs. Needed for SQS relationship.*/
char* server_address; /*The server domain name or IP address. Needed for
MQBROKER relationship.*/
} nr_segment_message_params_t;

/*
Expand Down
4 changes: 0 additions & 4 deletions axiom/nr_segment_private.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,7 @@ void nr_segment_message_destroy_fields(nr_segment_message_t* message) {

nr_free(message->destination_name);
nr_free(message->messaging_system);
nr_free(message->cloud_region);
nr_free(message->cloud_account_id);
nr_free(message->cloud_resource_id);
nr_free(message->server_address);
nr_free(message->aws_operation);
}

void nr_segment_destroy_typed_attributes(
Expand Down
Loading

0 comments on commit b342fc5

Please sign in to comment.