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.

Sign up for GitHub

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

Merged
merged 4 commits into from
May 7, 2025
+5 −0
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
936bc36
fix: add missing semantic conventions require in AWS resource detectors
yiyuan-he May 2, 2025
e54f72f
fix: add semantic conventions require to top-level file
yiyuan-he May 2, 2025
c150b05
Merge branch 'main' into fix-missing-import
yiyuan-he May 2, 2025
358fde7
Merge branch 'main' into fix-missing-import
kaylareopelle May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions resources/aws/lib/opentelemetry-resource-detector-aws.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@
#
# SPDX-License-Identifier: Apache-2.0

require 'opentelemetry/semantic_conventions/resource'
require_relative 'opentelemetry/resource/detector'
1 change: 1 addition & 0 deletions resources/aws/lib/opentelemetry/resource/detector/aws/ec2.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
require 'net/http'
require 'json'
require 'opentelemetry/common'
require 'opentelemetry/semantic_conventions/resource'

module OpenTelemetry
module Resource
Expand Down
1 change: 1 addition & 0 deletions resources/aws/lib/opentelemetry/resource/detector/aws/ecs.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
require 'json'
require 'socket'
require 'opentelemetry/common'
require 'opentelemetry/semantic_conventions/resource'

module OpenTelemetry
module Resource
Expand Down
2 changes: 2 additions & 0 deletions resources/aws/lib/opentelemetry/resource/detector/aws/lambda.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


module OpenTelemetry
module Resource
module Detector
Expand Down
Loading