Improve detection of resource attributes on ECS #4574

felixscheinost · 2022-06-30T16:14:13Z

This improves the detection of resource attributes on ECS by fetching ECS metadata from ECS_CONTAINER_METADATA_URI or ECS_CONTAINER_METADATA_URI_V4.

Previously only CONTAINER_NAME and CONTAINER_ID id were set.

Now we set:

CONTAINER_ID
CONTAINER_NAME
AWS_ECS_CONTAINER_ARN
CONTAINER_IMAGE_NAME
CONTAINER_IMAGE_TAG
aws.ecs.container.image.id
AWS_LOG_GROUP_ARNS
AWS_LOG_GROUP_NAMES
AWS_LOG_STREAM_NAMES
AWS_ECS_TASK_ARN
AWS_ECS_TASK_FAMILY
AWS_ECS_TASK_REVISION

Especially AWS_LOG_GROUP_ARNS is important so that connection of traces to logs works OOTB on X-Ray.

codecov · 2022-06-30T16:26:00Z

Codecov Report

Base: 90.04% // Head: 90.60% // Increases project coverage by +0.56% 🎉

Coverage data is based on head (1fc24b4) compared to base (0860b38).
Patch has no changes to coverable lines.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4574      +/-   ##
============================================
+ Coverage     90.04%   90.60%   +0.56%     
+ Complexity     5057     4906     -151     
============================================
  Files           582      567      -15     
  Lines         15580    14728     -852     
  Branches       1495     1414      -81     
============================================
- Hits          14029    13345     -684     
+ Misses         1096      959     -137     
+ Partials        455      424      -31

Impacted Files	Coverage Δ
...java/io/opentelemetry/sdk/trace/data/SpanData.java	`75.00% <0.00%> (-25.00%)`	⬇️
...esting/context/SettableContextStorageProvider.java	`37.93% <0.00%> (-24.14%)`	⬇️
...a/io/opentelemetry/sdk/trace/internal/JcTools.java	`28.57% <0.00%> (-4.77%)`	⬇️
...ernal/aggregator/ExplicitBucketHistogramUtils.java	`97.29% <0.00%> (-2.71%)`	⬇️
.../opentelemetry/sdk/internal/ComponentRegistry.java	`90.90% <0.00%> (-2.43%)`	⬇️
...o/opentelemetry/api/internal/ReadOnlyArrayMap.java	`91.11% <0.00%> (-2.23%)`	⬇️
...opentelemetry/opentracingshim/SpanBuilderShim.java	`77.89% <0.00%> (-0.68%)`	⬇️
...n/java/io/opentelemetry/sdk/logs/LogProcessor.java	`85.71% <0.00%> (ø)`
.../java/io/opentelemetry/sdk/logs/SdkLogEmitter.java	`100.00% <0.00%> (ø)`
...va/io/opentelemetry/sdk/logs/NoopLogProcessor.java	`100.00% <0.00%> (ø)`
... and 160 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

anuraaga

Thanks! This generally looks good, @willarmiros can you also give it a check?

anuraaga · 2022-07-01T02:32:40Z

sdk-extensions/aws/src/main/java/io/opentelemetry/sdk/extension/aws/resource/EcsResource.java


-    AttributesBuilder attrBuilders = Attributes.builder();
+  static void parseUrl(SimpleHttpClient httpClient, String url, AttributesBuilder attrBuilders) {


Suggested change

static void parseUrl(SimpleHttpClient httpClient, String url, AttributesBuilder attrBuilders) {

static void fetchMetadata(SimpleHttpClient httpClient, String url, AttributesBuilder attrBuilders) {

This improves the detection of resource attributes on ECS by fetching ECS metadata from `ECS_CONTAINER_METADATA_URI` or `ECS_CONTAINER_METADATA_URI_V4`. Previously only `CONTAINER_NAME` and `CONTAINER_ID` id were set. Now we set: - CONTAINER_ID - CONTAINER_NAME - AWS_ECS_CONTAINER_ARN - CONTAINER_IMAGE_NAME - CONTAINER_IMAGE_TAG - aws.ecs.container.image.id - AWS_LOG_GROUP_ARNS - AWS_LOG_GROUP_NAMES - AWS_LOG_STREAM_NAMES - AWS_ECS_TASK_ARN - AWS_ECS_TASK_FAMILY - AWS_ECS_TASK_REVISION Especially AWS_LOG_GROUP_ARNS is important so that connection of traces to logs works OOTB on X-Ray.

willarmiros · 2022-07-06T17:00:40Z

Thanks so much for this improvement! I will have someone from AWS take a look at this shortly

github-actions · 2022-07-20T17:33:05Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

felixscheinost · 2022-07-27T07:04:16Z

Bump: Please don't close this PR automatically.

jkwatson · 2022-07-27T13:36:19Z

Thanks so much for this improvement! I will have someone from AWS take a look at this shortly

@willarmiros any update here?

willarmiros · 2022-08-09T16:55:44Z

Apologies for the delay here, we will be having someone review this shortly!

mmanciop · 2022-08-09T18:42:32Z

@felixscheinost i missed out on this PR and worked on #4670. Your PR is effectively a superset of mine, with the exception of AWS_ECS_LAUNCHTYPE and AWS_LOGS_STREAM_ARNS and a couple small bugs in the generation of the AWS Log Group ARN. I prepared a fix for your PR in https://github.com/lumigo-io/opentelemetry-java/tree/pr-for-felix. @willarmiros is going to have a look at these PRs in the next few days. Would you be interested in pulling into your PR my changes? :-)

felixscheinost · 2022-08-12T08:58:03Z

@mmanciop Thanks for rebasing your changes on my PR!

Regarding the change in the log group ARN. That dosn't seem right to me. Have you tested this on AWS?

I just changed this line on my branch, to test this change in isolation, and deployed to our test evironment.

// from
attributesBuilder.put(ResourceAttributes.AWS_LOG_GROUP_ARNS, "arn:aws:logs:" + region + ":" + account + ":log-group:" + logGroupName);
// to
attributesBuilder.put(ResourceAttributes.AWS_LOG_GROUP_ARNS, "arn:aws:logs:" + region + ":" + account + ":log-group:/aws" + logGroupName + ":*");

With this change in place I can no longer see the logs for a trace in CloudWatch.

If I view a trace, for which I previously could view logs, and click on "Resources" for "Cloudwatch logs" I see the following:

[
  {
    "log_group": "*",
    "arn": "arn:aws:logs:eu-central-1:<account>:log-group:/aws<the name of my log group>:*"
  }
]

I also tried adding another separting / between /aws and logGroupName but that also doesn't work. In CloudWatch I then see:

[
  {
    "log_group": "*",
    "arn": "arn:aws:logs:eu-central-1:<account>:log-group:/aws/<the name of my log group>:*"
  }
]

But that is also wrong as my log group is just called <the name of my log group> and not /aws/<the name of my log group>.

mmanciop · 2022-08-12T12:13:26Z

@mmanciop Thanks for rebasing your changes on my PR!

Regarding the change in the log group ARN. That dosn't seem right to me. Have you tested this on AWS?

I just changed this line on my branch, to test this change in isolation, and deployed to our test evironment.
// from
attributesBuilder.put(ResourceAttributes.AWS_LOG_GROUP_ARNS, "arn:aws:logs:" + region + ":" + account + ":log-group:" + logGroupName);
// to
attributesBuilder.put(ResourceAttributes.AWS_LOG_GROUP_ARNS, "arn:aws:logs:" + region + ":" + account + ":log-group:/aws" + logGroupName + ":*");
With this change in place I can no longer see the logs for a trace in CloudWatch.

If I view a trace, for which I previously could view logs, and click on "Resources" for "Cloudwatch logs" I see the following:
[
  {
    "log_group": "*",
    "arn": "arn:aws:logs:eu-central-1:<account>:log-group:/aws<the name of my log group>:*"
  }
]
I also tried adding another separting / between /aws and logGroupName but that also doesn't work. In CloudWatch I then see:
[
  {
    "log_group": "*",
    "arn": "arn:aws:logs:eu-central-1:<account>:log-group:/aws/<the name of my log group>:*"
  }
]
But that is also wrong as my log group is just called <the name of my log group> and not /aws/<the name of my log group>.

I compared the ARNs with the output of aws logs describe-log-groups and aws logs describe-log-streams. At first, when I started investigating the change, I was as surprised as you. @willarmiros do you concur with these ARN values?

felixscheinost · 2022-08-12T14:39:21Z

@mmanciop When I do describe-log-groups on my log groups I get an ARN without /aws.

Only when the log group name contains /aws, like for lambdas, does the ARN also contain /aws.

mmanciop · 2022-08-12T15:27:33Z

@mmanciop When I do describe-log-groups on my log groups I get an ARN without /aws.

Only when the log group name contains /aws, like for lambdas, does the ARN also contain /aws.

I tried it on ECS, but this bears more validation. Brb :-)

mmanciop · 2022-08-12T15:37:48Z

So, what I see in the ECS console when deploying a simple CDK + Amazon ECS project is this:

awslogs-group | /aws/batch/job
-- | --
awslogs-region | eu-central-1
awslogs-stream-prefix | CrawlerJobDefinition4D5-55fb222b188f18e

The log-group is called /aws/batch/job in CloudWatch and the ARN is arn:aws:logs:eu-central-1:170906415945:log-group:/aws/batch/job:*.

The AWS CLI reports:

% aws logs describe-log-groups | jq '.logGroups[] | select(.logGroupName == "/aws/batch/job")'
{
  "logGroupName": "/aws/batch/job",
  "creationTime": 1658054662260,
  "metricFilterCount": 0,
  "arn": "arn:aws:logs:eu-central-1:170906415945:log-group:/aws/batch/job:*",
  "storedBytes": 141110571
}

So, yes, we should not add programmatically /aws. The :* at the end of the ARN for the log-group, however, seem pretty legit. Do you agree @felixscheinost ?

mmanciop · 2022-08-12T15:47:01Z

@felixscheinost I pushed a commit to remove the spurious /aws prefix from the ARNs. Thanks for catching that :-)

felixscheinost · 2022-08-12T17:04:12Z

The :* at the end of the ARN for the log-group, however, seem pretty legit.

Yeah, I think so. It seems like it does work without it but both the CLI and aws.amazon.com display log groups with that trailing :*.

Okay, I will manually test the other changes from your commit after the weekend and if everything looks fine (which I assume it will), pull them into this PR.

Is it okay for you if I squash your two commits into one and push it on top of this branch?

mmanciop · 2022-08-12T18:43:05Z

Is it okay for you if I squash your two commits into one and push it on top of this branch?

Yup, I think in the end it is best to squash the entire PR on merge anyhow

felixscheinost · 2022-08-17T06:45:18Z

Hi @mmanciop, unfortunately even the trailing :* seems to break showing the logs for a trace.

I cherry picked all your changes, deployed and tried showing the logs for a trace but didn't see any. As sometimes the logs take a while to show up I waited and tried again but still no logs.

Then I removed the trailing :* from the log group ARNs, deployed again and now I can see the logs again.

The output for the trace under "Resources" -> "CloudWatch logs" is:

[
  {
    "log_group": "*",
    "arn": "arn:aws:logs:eu-central-1:<account number>:log-group:<log group name>:*"
  }
]

vs this:

[
  {
    "log_group": "<log group name>",
    "arn": "arn:aws:logs:eu-central-1:<account number>:log-group:<log group name>"
  }
]

So it seems CloudWatch is very peculiar about how the log group ARNs are formatted. Maybe someone from AWS can verify this.

mmanciop · 2022-08-17T09:39:31Z

I cherry picked all your changes, deployed and tried showing the logs for a trace but didn't see any. As sometimes the logs take a while to show up I waited and tried again but still no logs.

Then I removed the trailing :* from the log group ARNs, deployed again and now I can see the logs again.

@felixscheinost how exactly are you using these ARNs? With which tool? Because the issue could be there too.

Besides, the spec clearly states with the example that the :* must be there: https://opentelemetry.io/docs/reference/specification/resource/semantic_conventions/cloud_provider/aws/logs/

felixscheinost · 2022-08-17T10:18:10Z

I am using CloudWatch traces on the official AWS web console.

felixscheinost · 2022-08-17T10:36:06Z

Besides, the spec clearly states with the example that the :* must be there: https://opentelemetry.io/docs/reference/specification/resource/semantic_conventions/cloud_provider/aws/logs/

These are merely "examples".

This links also references the official AWS documentation, which states:

Both of the following are used. The second one, with the * at the end, is what is returned by the describe-log-groups CLI command and the DescribeLogGroups API.
arn:aws:logs:region:account-id:log-group:log_group_name
arn:aws:logs:region:account-id:log-group:log_group_name:*

I think in the end what works is what's correct. 3rd party tools which parse aws.log.group.arns can be adapted. AWS CloudWatch can hardly be adapted.

mmanciop · 2022-08-17T11:19:07Z

I suppose @willarmiros, @Aneurysm9 should be making the call and, if :* has to go, update the examples in the spec. However, considering that in CloudWatch actually is showing even in the console ARNs with the :*, I think it is a bug that X-Ray should fix.

willarmiros · 2022-08-19T15:43:08Z

Sorry for being late here: so from my reading there were 2 issues:

/aws was being pre-pended to log groups in the resource detector, but that was fixed to no longer do that (which is the correct fix)
There is a question of whether to include the trailing :* in the log group ARN when recording it on the trace. It seems like the expected behavior is "it doesn't matter" because both such ARN formats are valid, however in practice only the ARN without the trailing :* works. I have just found the bug on our end here: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/8a821bb7829a23924edd2e23b22c6751277ab2ea/exporter/awsxrayexporter/internal/translator/aws.go#L272-L279

I will submit a fix for the bug so that both ARN formats work. The spec should stay as-is because the examples are still valid formats, just our parsing code is wrong. However, for now it would be best if the resource detectors could record the ARNs without the trailing :* so that everything still works while we wait for the fix to land.

jj22ee · 2022-08-23T06:39:57Z

...xtensions/aws/src/test/java/io/opentelemetry/sdk/extension/aws/resource/EcsResourceTest.java

    assertThat(attributes)
        .containsOnly(
            entry(ResourceAttributes.CLOUD_PROVIDER, "aws"),
            entry(ResourceAttributes.CLOUD_PLATFORM, "aws_ecs"),
-            entry(ResourceAttributes.CONTAINER_NAME, InetAddress.getLocalHost().getHostName()));
+            entry(ResourceAttributes.CONTAINER_NAME, "ecs-curltest-24-curl-cca48e8dcadd97805600"),


Change "24" to "26" for consistency.

jj22ee · 2022-08-23T06:40:17Z

sdk-extensions/aws/src/test/resources/ecs-container-metadata-v4.json

+{
+  "DockerId": "ea32192c8553fbff06c9340478a2ff089b2bb5646fb718b4ee206641c9086d66",
+  "Name": "curl",
+  "DockerName": "ecs-curltest-24-curl-cca48e8dcadd97805600",


Change "24" to "26" for consistency.

jj22ee · 2022-08-23T06:40:21Z

sdk-extensions/aws/src/test/resources/ecs-container-metadata-v4.json

+    "com.amazonaws.ecs.container-name": "curl",
+    "com.amazonaws.ecs.task-arn": "arn:aws:ecs:us-west-2:111122223333:task/default/8f03e41243824aea923aca126495f665",
+    "com.amazonaws.ecs.task-definition-family": "curltest",
+    "com.amazonaws.ecs.task-definition-version": "24"


Should be "26" to match ecs-task-metadata-v4.json

jj22ee

Looks good to me!
Just requested a small change to keep the task-definition-version number consistent in the V4 files.

willarmiros · 2022-08-29T23:12:32Z

Hi @mmanciop - I have opened the PR to address the :* issue on the X-Ray exporter side, and @jj22ee has left a few comments but approved. Would you mind addressing the comments so we can then merge?

mmanciop · 2022-08-30T07:11:36Z

@willarmiros I cannot push to @felixscheinost's branch (I have to push rights for his fork, which he used to open this PR), but I prepared the commit on https://github.com/lumigo-io/opentelemetry-java/tree/pr-for-felix .

`CONTAINER_NAME` and `AWS_ECS_TASK_REVISION` should match

This commit adds implementations for the `aws.ecs.launchtype` and `aws.logs.stream.arns` attributes, as well as fixing the generation of log group ARNs.

felixscheinost · 2022-08-30T07:45:46Z

I addressed @jj22ee's comments and cherry-picked @mmanciop's additions.

felixscheinost · 2022-08-30T07:56:01Z

@willarmiros

However, for now it would be best if the resource detectors could record the ARNs without the trailing :* so that everything still works while we wait for the fix to land.

I pushed another commit which removes the trailing :* for now.

Both with and without trailing `:*` are valid formats but there is a bug in the OpenTelementry collector which can’t handle the trailing `:*` (for now) (see open-telemetry/opentelemetry-collector-contrib#13702) So remove addition of the trailing `:*` for now.

jkwatson · 2022-09-05T16:43:14Z

@willarmiros when you think this is ready, please approve and we can move forward. Thanks!

jkwatson

Thanks!

felixscheinost requested review from anuraaga, jack-berg, jkwatson and jsuereth as code owners June 30, 2022 16:14

felixscheinost requested a review from a user June 30, 2022 16:14

felixscheinost requested a review from Oberon00 as a code owner June 30, 2022 16:14

felixscheinost force-pushed the feature/improve-ecs-resource branch 2 times, most recently from e9ebf1e to 46fbf18 Compare June 30, 2022 16:21

anuraaga reviewed Jul 1, 2022

View reviewed changes

felixscheinost force-pushed the feature/improve-ecs-resource branch from 46fbf18 to d0363a2 Compare July 1, 2022 07:39

github-actions bot added the Stale label Jul 20, 2022

github-actions bot removed the Stale label Jul 27, 2022

willarmiros mentioned this pull request Aug 9, 2022

Implement aws.ecs.* resource attributes #4670

Closed

jj22ee reviewed Aug 23, 2022

View reviewed changes

jj22ee approved these changes Aug 23, 2022

View reviewed changes

This was referenced Aug 29, 2022

[exporter/awsxrayexporter] fixed cloudwatch log group ARN parsing open-telemetry/opentelemetry-collector-contrib#13700

Merged

Log Group Parsing bug in awsxrayexporter open-telemetry/opentelemetry-collector-contrib#13702

Closed

felixscheinost and others added 2 commits August 30, 2022 09:42

Change „24“ to „26“ in fixtures for consistency

9b0b6b4

`CONTAINER_NAME` and `AWS_ECS_TASK_REVISION` should match

Implement a few more attributes, fix ARNs

57cd334

This commit adds implementations for the `aws.ecs.launchtype` and `aws.logs.stream.arns` attributes, as well as fixing the generation of log group ARNs.

felixscheinost force-pushed the feature/improve-ecs-resource branch from 27edbd8 to 1fc24b4 Compare August 30, 2022 08:01

willarmiros approved these changes Sep 6, 2022

View reviewed changes

jkwatson approved these changes Sep 7, 2022

View reviewed changes

jack-berg merged commit b5ef538 into open-telemetry:main Sep 8, 2022

mackjmr mentioned this pull request Jan 31, 2023

WithContainerID: Extracts incorrect container ID in ECS. open-telemetry/opentelemetry-go#3633

Closed

jj22ee mentioned this pull request Nov 28, 2024

REQUEST: New membership for @jj22ee open-telemetry/community#2468

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve detection of resource attributes on ECS #4574

Improve detection of resource attributes on ECS #4574

felixscheinost commented Jun 30, 2022

codecov bot commented Jun 30, 2022 •

edited

Loading

anuraaga left a comment

anuraaga Jul 1, 2022

willarmiros commented Jul 6, 2022

github-actions bot commented Jul 20, 2022

felixscheinost commented Jul 27, 2022

jkwatson commented Jul 27, 2022

willarmiros commented Aug 9, 2022

mmanciop commented Aug 9, 2022 •

edited

Loading

felixscheinost commented Aug 12, 2022 •

edited

Loading

mmanciop commented Aug 12, 2022

felixscheinost commented Aug 12, 2022 •

edited

Loading

mmanciop commented Aug 12, 2022

mmanciop commented Aug 12, 2022

mmanciop commented Aug 12, 2022

felixscheinost commented Aug 12, 2022

mmanciop commented Aug 12, 2022

felixscheinost commented Aug 17, 2022

mmanciop commented Aug 17, 2022 •

edited

Loading

felixscheinost commented Aug 17, 2022

felixscheinost commented Aug 17, 2022

mmanciop commented Aug 17, 2022 •

edited

Loading

willarmiros commented Aug 19, 2022 •

edited

Loading

jj22ee Aug 23, 2022

jj22ee Aug 23, 2022

jj22ee Aug 23, 2022

jj22ee left a comment

willarmiros commented Aug 29, 2022

mmanciop commented Aug 30, 2022

felixscheinost commented Aug 30, 2022

felixscheinost commented Aug 30, 2022

jkwatson commented Sep 5, 2022

jkwatson left a comment


		AttributesBuilder attrBuilders = Attributes.builder();
		static void parseUrl(SimpleHttpClient httpClient, String url, AttributesBuilder attrBuilders) {

	static void parseUrl(SimpleHttpClient httpClient, String url, AttributesBuilder attrBuilders) {
	static void fetchMetadata(SimpleHttpClient httpClient, String url, AttributesBuilder attrBuilders) {

Improve detection of resource attributes on ECS #4574

Improve detection of resource attributes on ECS #4574

Conversation

felixscheinost commented Jun 30, 2022

codecov bot commented Jun 30, 2022 • edited Loading

Codecov Report

anuraaga left a comment

Choose a reason for hiding this comment

anuraaga Jul 1, 2022

Choose a reason for hiding this comment

willarmiros commented Jul 6, 2022

github-actions bot commented Jul 20, 2022

felixscheinost commented Jul 27, 2022

jkwatson commented Jul 27, 2022

willarmiros commented Aug 9, 2022

mmanciop commented Aug 9, 2022 • edited Loading

felixscheinost commented Aug 12, 2022 • edited Loading

mmanciop commented Aug 12, 2022

felixscheinost commented Aug 12, 2022 • edited Loading

mmanciop commented Aug 12, 2022

mmanciop commented Aug 12, 2022

mmanciop commented Aug 12, 2022

felixscheinost commented Aug 12, 2022

mmanciop commented Aug 12, 2022

felixscheinost commented Aug 17, 2022

mmanciop commented Aug 17, 2022 • edited Loading

felixscheinost commented Aug 17, 2022

felixscheinost commented Aug 17, 2022

mmanciop commented Aug 17, 2022 • edited Loading

willarmiros commented Aug 19, 2022 • edited Loading

jj22ee Aug 23, 2022

Choose a reason for hiding this comment

jj22ee Aug 23, 2022

Choose a reason for hiding this comment

jj22ee Aug 23, 2022

Choose a reason for hiding this comment

jj22ee left a comment

Choose a reason for hiding this comment

willarmiros commented Aug 29, 2022

mmanciop commented Aug 30, 2022

felixscheinost commented Aug 30, 2022

felixscheinost commented Aug 30, 2022

jkwatson commented Sep 5, 2022

jkwatson left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 30, 2022 •

edited

Loading

mmanciop commented Aug 9, 2022 •

edited

Loading

felixscheinost commented Aug 12, 2022 •

edited

Loading

felixscheinost commented Aug 12, 2022 •

edited

Loading

mmanciop commented Aug 17, 2022 •

edited

Loading

mmanciop commented Aug 17, 2022 •

edited

Loading

willarmiros commented Aug 19, 2022 •

edited

Loading