Skip to content

fix: add missing semantic conventions require in AWS resource detectors #1520

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yiyuan-he
Copy link
Contributor

@yiyuan-he yiyuan-he commented May 2, 2025

What does this pull request do?

Fixes an issue where AWS resource detectors (EC2, ECS, Lambda) were using OpenTelemetry::SemanticConventions::Resource without requiring the module that defines it. This caused "uninitialized constant" errors when using these detectors in production environments where the semantic conventions module wasn't already loaded.

This PR implements a dual strategy for dependency management:

  1. Added explicit require 'opentelemetry/semantic_conventions/resource' statements to each AWS detector file to ensure proper dependency loading regardless of the runtime environment.
  2. Maintained these individual requires while also adding the require statement to the top-level detector file.

This approach ensures that we are explicit about the following behavior:

  • When the gem is required normally via require 'opentelemetry-resource-detector-aws', all dependencies are loaded properly.
  • When individual detectors are required directly (e.g., require 'opentelemetry/resource/detector/aws/ec2'), they still have all their necessary dependencies.

Issues

#1515

@@ -4,6 +4,8 @@
#
# SPDX-License-Identifier: Apache-2.0

require 'opentelemetry/semantic_conventions/resource'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any disadvantage to requiring this dependency once at the top level?

resources/aws/lib/opentelemetry-resource-detector-aws.rb

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should be fine to include in the top level.

I was testing through require 'opentelemetry/resource/detector/aws/ec2', and I got the issue.

Actually, if I do require 'opentelemetry-resource-detector-aws', then it doesn't need require semantic_conventions/resource (e.g. no uninitialized constant error).

I guess I should require the detector through the gem name, not gem file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think our instructions on opentelemetry.io are also a bit misleading.

In most cases bundler will require the root of the gem and that should be enough to start the require process of all of the files down the tree so we should encourage requiring top levels files as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Ariel and Xuan for the additional context and feedback. I'll push a commit to require the dependency at resources/aws/lib/opentelemetry-resource-detector-aws.rb instead.

However, i'm curious if we should move forward with these changes and implicitly support this unrecommended dependency import method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The top level directory requires the opentelemetry/sdk which requires the transitive dependencies:

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/resources/aws/lib/opentelemetry/resource/detector.rb#L7

https://github.com/open-telemetry/opentelemetry-ruby/blob/main/sdk/lib/opentelemetry/sdk.rb#L10

Is your preference that all of these dependencies be explicitly required?

Is that because each sub detector may be used in isolation without loading the other detectors?

If so, then for consistency I think you all should treat each as their own nested library and explicitly require all dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Ariel - that's helpful context. Yes, the intention is to allow sub-detectors to be used in isolation. I'll update the PR to explicitly require all dependencies for consistency across detectors. This should make it clearer for users which dependencies are needed when using specific detectors independently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR to add the semantic conventions require to the top-level file while maintaining the individual requires in each detector file.

This should ensure:

  1. When the gem is required normally via require 'opentelemetry-resource-detector-aws', all dependencies are loaded properly.
  2. When individual detectors are required directly (e.g., require'opentelemetry/resource/detector/aws/ec2'), they still have all their necessary dependencies.

Added some notes to the PR description as well for documentation purposes.

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.

3 participants