Add ECS metadata allowing cloudwatch-logs to be linked with traces #93

etiennechabert · 2023-09-11T08:12:15Z

In this PR

For ECS/Fargate: allow traces and logs to be linked together by adding missing log-group information

Warning

It will only work for ECS Task running on Fargate 1.4 and above.

Related issue

#90

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

etiennechabert · 2023-09-11T10:32:22Z

lib/aws-xray-sdk/plugins/ecs.rb

@@ -10,13 +10,80 @@ module ECS

      ORIGIN = 'AWS::ECS::Container'.freeze

+      # Only compatible with v4!
+      # The v3 metadata url does not contain cloudwatch informations
+      METADATA_ENV_KEY = 'ECS_CONTAINER_METADATA_URI_V4'


Here we have a strong dependency with Fargate 1.4, otherwise this environment variable will not be defined:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-metadata-endpoint-v4-fargate.html

Sadly the necessary information are not exposed with the meta-information of the v3:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-metadata-endpoint-v3-fargate.html

etiennechabert · 2023-09-11T10:56:54Z

lib/aws-xray-sdk/plugins/ecs.rb

+          ecs: {
+            container_arn: data['ContainerARN'],
+          },
+          cloudwatch_logs: {
+            log_group: data["LogOptions"]['awslogs-group'],
+            log_region: data["LogOptions"]['awslogs-region'],
+            arn: data['ContainerARN']
+          }
+        }


Potentially, much more could be added to the x-ray segment.

This particular information for example would be valuable to hunt regressions:

com.amazonaws.ecs.task-definition-version

etiennechabert · 2023-09-11T10:57:57Z

test/aws-xray-sdk/tc_plugin.rb

@@ -43,6 +44,7 @@ def test_ec2_metadata_v2_successful
        ami_id: 'ami-03cca83dd001d4666'
      }
    }
+    # We should probably use `assert_equal` here ? Always true otherwise...
    assert expected, XRay::Plugins::EC2.aws


Not related to my PR, but assert is always returning true here... regardless what is passed

We probably want to use assert_equal if we want to compare something expected with XRay::Plugins::EC2.aws

etiennechabert · 2023-09-11T10:58:38Z

test/aws-xray-sdk/tc_plugin.rb

+      "LogOptions"=>{"awslogs-group"=>"/ecs/a_service_name", "awslogs-region"=>"eu-central-1", "awslogs-stream"=>"ecs/a_service_name/a_task_id"},
+    }
+
+    ENV[XRay::Plugins::ECS::METADATA_ENV_KEY] = dummy_metadata_uri


I tried a few things to mutate the ENV in a better way...

Could not do it without adding extra dependencies to the project

etiennechabert · 2023-09-11T11:03:10Z

test/aws-xray-sdk/tc_plugin.rb

+        container: Socket.gethostname,
+        container_arn: 'arn:aws:ecs:eu-central-1:an_id:container/a_cluster/a_cluster_id/a_task_id',
+      },
+      cloudwatch_logs: {:log_group=>"/ecs/a_service_name", :log_region=>"eu-central-1", :arn=>"arn:aws:ecs:eu-central-1:an_id:container/a_cluster/a_cluster_id/a_task_id"}


This is the key change here, with this information present in the trace, the logs should be linked

etiennechabert · 2023-09-11T11:18:22Z

lib/aws-xray-sdk/plugins/ecs.rb

-    # Due to lack of ECS container metadata service, the only host information
-    # available is the host name.


This comment is not anymore relevant

mxiamxia

LGTM! Thanks for the contribution!

* Add ECS metadata allowing cloudwatch-logs to be linked with traces (aws#93) * First step * Add comment * Typo * Copy logic of EC2 plugin for ECS * Typo * Back to symbols * Copy paste all EC2 tests as starting point for ECS * Comments * Somewhat working ? * Backword compatible with prevous feature when not 1.4 * Add hint for fargate 1.4 in case of errors --------- Co-authored-by: Etienne Chabert <[email protected]> * release 0.15.0 (aws#94) * Update version for 0.15.0 release (aws#95) * Update continuous-build.yml (aws#97) the latest macos-14 supports arm only. This PR using the previous version of macos image to unblock the release. * Fix issue: Wrong duration for DB transaction event on ROR 7.1 aws#92 (aws#96) * Fix issue: Wrong duration for DB transaction event on ROR 7.1 aws#92 * Added unit tests and restructured the code * Fix unit tests * Update continuous-build.yml (aws#97) the latest macos-14 supports arm only. This PR using the previous version of macos image to unblock the release. * Worked on the jj22ee's comment and added logic that convert in seconds only for rails 7.1 versions. * Conditionally handle for ruby 7.1 --------- Co-authored-by: Lei Wang <[email protected]> * Release 0.16.0 (aws#98) --------- Co-authored-by: Chabert Etienne <[email protected]> Co-authored-by: Etienne Chabert <[email protected]> Co-authored-by: Prashant Srivastava <[email protected]> Co-authored-by: Lei Wang <[email protected]> Co-authored-by: Osama Bin Junaid <[email protected]> Co-authored-by: Min Xia <[email protected]>

Etienne Chabert added 2 commits September 11, 2023 10:09

First step

8d428d5

Add comment

170a226

etiennechabert requested a review from a team as a code owner September 11, 2023 08:12

etiennechabert marked this pull request as draft September 11, 2023 08:12

Etienne Chabert added 7 commits September 11, 2023 10:12

Typo

318026c

Copy logic of EC2 plugin for ECS

3b970f0

Typo

64159e1

Back to symbols

3064c60

Copy paste all EC2 tests as starting point for ECS

74037ec

Comments

de3600b

Somewhat working ?

3f8c871

etiennechabert commented Sep 11, 2023

View reviewed changes

Backword compatible with prevous feature when not 1.4

730ccb7

etiennechabert commented Sep 11, 2023

View reviewed changes

etiennechabert marked this pull request as ready for review September 11, 2023 10:59

etiennechabert commented Sep 11, 2023

View reviewed changes

etiennechabert mentioned this pull request Sep 11, 2023

cloudwatch_logs properties aren't populated causing CloudWatch Tracer logs to not work #90

Closed

Add hint for fargate 1.4 in case of errors

b252f35

etiennechabert commented Sep 11, 2023

View reviewed changes

etiennechabert changed the title ~~Add cloudwatch logs to aws context in traces~~ Add ecs metadata regarding logs to traces Sep 11, 2023

etiennechabert changed the title ~~Add ecs metadata regarding logs to traces~~ Add ECS metadata allowing cloudwatch-logs to be linked with traces Sep 11, 2023

mxiamxia approved these changes Sep 25, 2023

View reviewed changes

srprash approved these changes Sep 25, 2023

View reviewed changes

srprash merged commit 75d350d into aws:master Sep 25, 2023
8 checks passed

etiennechabert deleted the add-cloudwatch_logs-to-aws-context-in-traces branch September 25, 2023 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ECS metadata allowing cloudwatch-logs to be linked with traces #93

Add ECS metadata allowing cloudwatch-logs to be linked with traces #93

etiennechabert commented Sep 11, 2023 •

edited

Loading

etiennechabert Sep 11, 2023

etiennechabert Sep 11, 2023

etiennechabert Sep 11, 2023

etiennechabert Sep 11, 2023

etiennechabert Sep 11, 2023

etiennechabert Sep 11, 2023 •

edited

Loading

mxiamxia left a comment

		# Due to lack of ECS container metadata service, the only host information
		# available is the host name.

Add ECS metadata allowing cloudwatch-logs to be linked with traces #93

Add ECS metadata allowing cloudwatch-logs to be linked with traces #93

Conversation

etiennechabert commented Sep 11, 2023 • edited Loading

In this PR

Warning

Related issue

etiennechabert Sep 11, 2023

Choose a reason for hiding this comment

etiennechabert Sep 11, 2023

Choose a reason for hiding this comment

etiennechabert Sep 11, 2023

Choose a reason for hiding this comment

etiennechabert Sep 11, 2023

Choose a reason for hiding this comment

etiennechabert Sep 11, 2023

Choose a reason for hiding this comment

etiennechabert Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

mxiamxia left a comment

Choose a reason for hiding this comment

etiennechabert commented Sep 11, 2023 •

edited

Loading

etiennechabert Sep 11, 2023 •

edited

Loading