Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Embed fargate data in container samples #50

Merged
merged 5 commits into from
Nov 12, 2020
Merged

Embed fargate data in container samples #50

merged 5 commits into from
Nov 12, 2020

Conversation

roobre
Copy link
Contributor

@roobre roobre commented Nov 9, 2020

Fixes #49

When running in transparent mode, as is the case in Fargate, the agent will not decorate metrics with Fargate metadata such as awsRegion, ecsLaunchType, and ecsClusterName. Breaking queries that use such attributes.

In this PR we fix this by pulling such metadata from the docker container labels that Fargate adds to the container information, by parsing the task arn and the cluster name. This is the same way in which https://github.com/newrelic/nri-ecs obtains this information. RegionFromTask and ResourceNameAndARNBase have been pulled from said repo as well.

TODO:

  • Unit testing

Manual testing checklist

  • Integration works as expected in Fargate mode, when FARGATE and NRIA_IS_FORWARD_ONLY are set
  • Integration works in as expected in EC2 mode
    json-comparison.tar.gz

@CLAassistant
Copy link

CLAassistant commented Nov 9, 2020

CLA assistant check
All committers have signed the CLA.

@roobre roobre force-pushed the santalla/fix-49 branch 3 times, most recently from 58b0bc2 to fdecd66 Compare November 10, 2020 14:13
@roobre roobre changed the title WIP: Echo Fargate metadata as metrics Embed fargate data in container samples Nov 10, 2020
Copy link
Contributor

@gsanchezgavier gsanchezgavier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good thanks for adding the test

// resourceNameAndARNBase explodes a resource ARN and returns the resource's name and the base ARN prefix for the
// account and region of the original resource.
func resourceNameAndARNBase(resourceARN string) (resourceName string, arnPrefix string) {
arnPrefixAndType := resourceARN[:strings.Index(resourceARN, "/")-1]
Copy link
Member

@paologallinaharbur paologallinaharbur Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the strings.Split function instead as in the previous method?
Only to avoid splitting using indexes directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both functions were taken (i.e. copy-pasted) from nri-ecs, and were left untouched for consistency even when I do not particularly like the implementation of either. The one that uses split, for example, does not perform any kind of bound-checking and relies on the passed string to be an arn.

If we are ok with changing them, I'd vote for rewriting both of them to use strings.Split and proper bound checking. What do you think?

Copy link
Member

@paologallinaharbur paologallinaharbur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a small comments, I believe it is fine, but if we can I would use the standard function 👍
Well done!

@roobre
Copy link
Contributor Author

roobre commented Nov 11, 2020

Following a conversation with @paologallinaharbur, we decided that the best approach for now would be to include the aws sdk even for this tiny bit (arn parsing), and file an issue to switch to said SDK for everything else (hopefully removing all manually vendored files)

@roobre roobre merged commit 2c6d6e3 into master Nov 12, 2020
@roobre roobre deleted the santalla/fix-49 branch January 22, 2021 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker integration should know when it's running in Fargate
4 participants