Skip to content

pre-release change for working with ruby 3.0.0#196

Merged
xuan-cao-swi merged 1 commit into
mainfrom
7-0-0-prev1
Jun 10, 2025
Merged

pre-release change for working with ruby 3.0.0#196
xuan-cao-swi merged 1 commit into
mainfrom
7-0-0-prev1

Conversation

@xuan-cao-swi
Copy link
Copy Markdown
Contributor

Description

The change is for pre-release solarwinds_apm ruby 7.0.0.prev1

  1. copied the aws resource detector from upstream into local file so that it can work with ruby < 3.1.0.
  2. removed the test case related to aws resource detector since from 7.0.0, we will use upstream aws resource detector, and the logic is tested from upstream test case.
  3. relax the ruby version to 3.0.0 for this pre-release only

Test (if applicable)

Tested on workflow: verify_install and ad-hoc test from testbed.

@xuan-cao-swi xuan-cao-swi marked this pull request as ready for review June 10, 2025 01:25
@xuan-cao-swi xuan-cao-swi requested a review from a team as a code owner June 10, 2025 01:25
Copy link
Copy Markdown

@cleverchuk cleverchuk left a comment

Choose a reason for hiding this comment

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

LGTM! left some questions.

Comment thread lib/solarwinds_apm/support/resource_detector/aws/ec2.rb
Comment thread lib/solarwinds_apm/support/resource_detector/aws/ec2.rb
file.each_line do |line|
line = line.strip
# Look for container ID (64 chars) at the end of the line
return line[-CONTAINER_ID_LENGTH..] if line.length > CONTAINER_ID_LENGTH
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is it guaranteed that the first line that satisfies this constraint is actually a container id?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This upstream code was copied from otel-js. I can't guarantee first line that satisfies this constraint is actually a container id since I didn't do any test myself.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

it looks like a follow-on PR to validate that it's indeed a container id might be worth a shot?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

something like here

Comment thread lib/solarwinds_apm/support/resource_detector/aws/ecs.rb
@xuan-cao-swi xuan-cao-swi merged commit 6d87140 into main Jun 10, 2025
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants