Skip to content

[SDK] Avoid throwing for non-string process executable names - #4540

Open
ryux1 wants to merge 3 commits into
open-telemetry:mainfrom
ryux1:fix/resource-non-string-executable-name
Open

ryux1 wants to merge 3 commits into
open-telemetry:mainfrom
ryux1:fix/resource-non-string-executable-name

Conversation

@ryux1

@ryux1 ryux1 commented Sep 8, 2026

Copy link
Copy Markdown

Fixes #4535

Changes

  • read process.executable.name with nostd::get_if when deriving the fallback service name
  • fall back to unknown_service when the attribute exists with a non-string value
  • preserve unknown_service:<process.executable.name> for valid string values
  • add regression coverage for both paths

Validation

  • reproduced the original bad_variant_access failure with the new regression test before applying the fix
  • all 16 resource tests pass with C++14 under ABI v1
  • all 16 resource tests pass with C++14 under ABI v2
  • opentelemetry_resources builds with C++14 and -fno-exceptions
  • clang-format 18 dry-run passes

For significant contributions:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed (this PR does not change the public API)

Implementation, tests, and this description were prepared with OpenAI Codex assistance.

@ryux1
ryux1 requested a review from a team as a code owner September 8, 2026 04:42
@linux-foundation-easycla

linux-foundation-easycla Bot commented Sep 8, 2026

Copy link
Copy Markdown

CLA Not Signed

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.43%. Comparing base (e81d957) to head (2d99cec).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4540      +/-   ##
==========================================
+ Coverage   86.41%   86.43%   +0.02%     
==========================================
  Files         524      524              
  Lines       20435    20437       +2     
==========================================
+ Hits        17657    17662       +5     
+ Misses       2778     2775       -3     
Files with missing lines Coverage Δ
sdk/src/resource/resource.cc 97.23% <100.00%> (+3.11%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ryux1
ryux1 force-pushed the fix/resource-non-string-executable-name branch from d86d10a to e88afb7 Compare September 8, 2026 14:08

@owent owent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM after CLA is signed.

@marcalff marcalff added the pr:waiting-on-cla Waiting on CLA label Sep 9, 2026
@marcalff

Copy link
Copy Markdown
Member

/easycla

@marcalff marcalff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

@marcalff marcalff changed the title Avoid throwing for non-string process executable names [SDK] Avoid throwing for non-string process executable names Sep 11, 2026
@marcalff

Copy link
Copy Markdown
Member

@ryux1

Thanks for the fix.

Please sign the EasyCLA, this is needed to merge.

@marcalff

Copy link
Copy Markdown
Member

/easycla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:waiting-on-cla Waiting on CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Resource::Create() throws bad_variant_access if process.executable.name isn't a string

5 participants