Skip to content

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

#
# SPDX-License-Identifier: Apache-2.0

require 'opentelemetry/semantic_conventions/resource'
Copy link
Contributor

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
Contributor

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
Contributor

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.

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thanks, @yiyuan-he!

@kaylareopelle kaylareopelle merged commit eae73d6 into open-telemetry:main May 7, 2025
62 checks passed
yiyuan-he added a commit to yiyuan-he/opentelemetry-ruby-contrib that referenced this pull request May 14, 2025
…rs (open-telemetry#1520)

* fix: add missing semantic conventions require in AWS resource detectors

* fix: add semantic conventions require to top-level file

---------

Co-authored-by: Kayla Reopelle <[email protected]>
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.

4 participants