-
Notifications
You must be signed in to change notification settings - Fork 196
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
base: main
Are you sure you want to change the base?
Conversation
@@ -4,6 +4,8 @@ | |||
# | |||
# SPDX-License-Identifier: Apache-2.0 | |||
|
|||
require 'opentelemetry/semantic_conventions/resource' |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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.
Added some notes to the PR description as well for documentation purposes.
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:
require 'opentelemetry/semantic_conventions/resource'
statements to each AWS detector file to ensure proper dependency loading regardless of the runtime environment.This approach ensures that we are explicit about the following behavior:
require 'opentelemetry-resource-detector-aws'
, all dependencies are loaded properly.require 'opentelemetry/resource/detector/aws/ec2'
), they still have all their necessary dependencies.Issues
#1515