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

Add AWS resource detectors to extension package #586

Merged

Conversation

NathanielRN
Copy link
Contributor

@NathanielRN NathanielRN commented Jul 15, 2021

Description

This PR adds Resource Detectors to automatically populate attributes under the resource namespace of every span. The included Resource Detectors are for the following AWS Service:

  • AWS Lambda
  • AWS Beanstalk
  • AWS EC2
  • AWS ECS
  • AWS EKS

This is similar to packages already available on OpenTelemetry JavaScript and OpenTelemetry Java.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • All ResourceDetectors have unit tests to verify file reading/API conformance
  • For all ResourceDetectors, I was able to run OTel Python on these services and verify that the spans are populated with the right attributes.
  • Beanstalk + EKS had additional non-obvious steps so for those I added links in the docstring!

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@NathanielRN NathanielRN requested review from a team, aabmass and ocelotl and removed request for a team July 15, 2021 00:38
@NathanielRN NathanielRN force-pushed the add-aws-resource-detectors-to-extension branch from 2a2f1e7 to a7d004a Compare July 15, 2021 00:40
@NathanielRN
Copy link
Contributor Author

NathanielRN commented Jul 15, 2021

Blocked on #585, converting to draft.

@NathanielRN NathanielRN marked this pull request as draft July 15, 2021 01:06
Copy link

@srprash srprash left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for doing this.

@NathanielRN NathanielRN requested a review from aabmass July 15, 2021 18:09
@NathanielRN NathanielRN force-pushed the add-aws-resource-detectors-to-extension branch from 7bbc574 to 2bb89dd Compare July 19, 2021 23:00
@NathanielRN NathanielRN marked this pull request as ready for review July 20, 2021 00:53
scripts/build.sh Outdated Show resolved Hide resolved
@@ -0,0 +1,31 @@
# Copyright The OpenTelemetry Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: opentelemetry.sdk.extension.aws.resources plural to match opentelemetry.sdk.resources? Not a big deal though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I didn't even notice that, actually I did resource because that is the folder it is in on the specifications

Also in java we put it under resource

And yet JavaScript, Go and PHP all put it under a folder called detectors/ 😅

I guess I'll defer to the SIG here, as GCP and other vendors will probably add their resource Detectors soon... maybe we should even move the SDK import path? 😓 Although I imagine that will be hard after 1.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

We aren't at 1.0 yet so nows the time to decide on an import path :D. But again, not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I meant we should move opentelemetry.sdk.resources to opentelemetry.sdk.resource to match the spec 🙂

I actually like .resource because there is only 1 resource: namespace in the attributes. Even if you add multiple ResourceDetectors you're still setting attributes in 1 namespace...

I'll leave it for now and if we need to we'll change it before this package goes 1.0 😄. Shouldn't be a big deal to do "both" if we need to as well later!

@NathanielRN NathanielRN requested a review from lzchen July 21, 2021 19:45
@NathanielRN NathanielRN force-pushed the add-aws-resource-detectors-to-extension branch from 79b9fb6 to cc84b34 Compare July 23, 2021 01:49

container_id = ""
try:
with open(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'll be better to catch this specific type of exception below than to have nested general exceptions caught.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand you correctly, you think it would be better than we catch FileNotFoundError specifically? That makes sense to me! I'll update it here and in the eks.py file.

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM

}
)
# pylint: disable=broad-except
except Exception as exception:
Copy link
Member

Choose a reason for hiding this comment

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

Hmm it would have been nice if this was handled in the ResourceDetector super class so the implementations didn't have to duplicate it every time, but too late now I think 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a really good point, do you mean something like this on opentelemetry-python-core?:

class ResourceDetector(abc.ABC):
    def __init__(self, raise_on_error=False):
        self.raise_on_error = raise_on_error

    @abc.abstractmethod
    def _detect(self) -> "Resource":
        raise NotImplementedError()

    def detect(self) -> "Resource":
        try:
            self._detect()
        except NotImplementedError as exception:
            raise exception
        # pylint: disable=broad-except
        except Exception as exception:
            if self.raise_on_error:
                raise exception
            
            logger.warning(f"{self.__class__.__name__} failed: {exception}")
            return Resource.get_empty()

I think that would definitely be worth it, maybe we can add it in a minor release for now and support (but deprecate) the existing method?

If this sounds like it makes sense I can open a small PR with this change on core! 🙂

except Exception as exception:
e_msg = f"{self.__class__.__name__} failed: {exception}"
if self.raise_on_error:
logger.exception(e_msg)
Copy link
Member

Choose a reason for hiding this comment

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

Is logging this redudant if we a re re-raising?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm that's a good point, I was hoping to add class name to the exception as e_msg = f"{self.__class__.__name__} failed: {exception}", but they can see it as been obvious enough from the Traceback.

Thanks! I'll update these exceptions across the detectors 🙂

@NathanielRN NathanielRN force-pushed the add-aws-resource-detectors-to-extension branch from 0f74f17 to 530a91b Compare July 23, 2021 22:14
@NathanielRN NathanielRN marked this pull request as ready for review July 23, 2021 22:23
@lzchen lzchen merged commit 1157eb2 into open-telemetry:main Jul 23, 2021
@NathanielRN NathanielRN deleted the add-aws-resource-detectors-to-extension branch February 15, 2022 21:41
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.

6 participants