fix(sdk): don't emit placeholder service.version in Composer detector#2004
Conversation
|
|
|
@open-telemetry/php-approvers small SDK fix for #1320 (Composer resource detector) with unit tests — review welcome when convenient. 🙏 |
When a Composer root package has no explicit version, Composer reports a placeholder pretty_version of `1.0.0+no-version-set`. The Composer resource detector passed this straight through as service.version, so applications that never set a version (e.g. the OTel demo) got a misleading `1.0.0+no-version-set` attribute. Other language SDKs do not set service.version by default. Skip the attribute when Composer reports the placeholder. An optional rootPackage constructor argument is added to make the branch testable. Fixes open-telemetry#1320 - Rollback: revert commit; restores unconditional service.version. Risk-Level: low AI-Agent: claude-opus-4-8
067fea1 to
1524d2f
Compare
|
/easycla |
| $attributes = [ | ||
| ServiceAttributes::SERVICE_NAME => InstalledVersions::getRootPackage()['name'], | ||
| ServiceAttributes::SERVICE_VERSION => InstalledVersions::getRootPackage()['pretty_version'], | ||
| ServiceAttributes::SERVICE_NAME => $rootPackage['name'] ?? null, |
There was a problem hiding this comment.
Should skip setting service.name if __root__.
There was a problem hiding this comment.
Done — now also skips service.name when the root package name is 'root'.
| private const UNSET_VERSION = '1.0.0+no-version-set'; | ||
|
|
||
| /** | ||
| * @param array{name?: string, pretty_version?: ?string}|null $rootPackage overrides |
There was a problem hiding this comment.
| * @param array{name?: string, pretty_version?: ?string}|null $rootPackage overrides | |
| * @param array{name: string, pretty_version: string, ...}|null $rootPackage overrides |
If we keep the parameter; to match InstalledVersions::getRootPackage().
IMO we should instead InstalledVersions::reload() the test data to keep the implementation clean.
There was a problem hiding this comment.
Reworked as suggested: dropped the $rootPackage constructor param and read InstalledVersions::getRootPackage() directly; tests now inject via InstalledVersions::reload(). Cleaner. Thanks!
…ge param Per review on open-telemetry#2004: also omit service.name when the Composer root package name is the placeholder '__root__'; remove the ?array $rootPackage constructor param and read InstalledVersions::getRootPackage() directly, with tests injecting data via InstalledVersions::reload().
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2004 +/- ##
============================================
+ Coverage 68.00% 68.07% +0.07%
- Complexity 3078 3080 +2
============================================
Files 459 459
Lines 9023 9025 +2
============================================
+ Hits 6136 6144 +8
+ Misses 2887 2881 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s Composer resource detector to avoid emitting misleading resource attributes when Composer reports placeholder metadata for the root package (notably service.version = 1.0.0+no-version-set), aligning behavior with other OpenTelemetry SDKs and addressing #1320.
Changes:
- Skip emitting
service.versionwhen Composer reports the1.0.0+no-version-setplaceholder. - Skip emitting
service.namewhen Composer reports the__root__placeholder name. - Add unit tests that simulate different root package metadata via
InstalledVersions::reload().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/SDK/Resource/Detectors/Composer.php | Adds placeholder constants and conditionally omits service.name/service.version when Composer reports default placeholder values. |
| tests/Unit/SDK/Resource/Detectors/ComposerTest.php | Adds tests covering explicit version emission and placeholder omission using InstalledVersions::reload() + reflection reset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $rootPackage = InstalledVersions::getRootPackage(); | ||
|
|
||
| $attributes = []; |
There was a problem hiding this comment.
Updated the PR description to match the implementation — the constructor-injection mention is gone; it now describes reading InstalledVersions::getRootPackage() directly with tests driven via InstalledVersions::reload(). Thanks.
Problem
Closes #1320.
When a Composer root package has no explicit
version, Composer substitutes the placeholder pretty version1.0.0+no-version-set(Composer\Package\RootPackage::DEFAULT_PRETTY_VERSION). TheComposerresource detector passedpretty_versionstraight through, so any application that never declares a version — e.g. the OpenTelemetry demo — ended up with a misleadingservice.version = 1.0.0+no-version-set.Likewise, when no package
nameis set Composer substitutes the placeholder__root__, which should not surface asservice.name.Other language SDKs do not set
service.versionby default, as discussed in the issue.Fix
service.versionattribute when Composer reports the1.0.0+no-version-setplaceholder.service.nameattribute when Composer reports the__root__placeholder.The detector reads
InstalledVersions::getRootPackage()directly (no constructor injection). Tests drive the placeholder paths by swapping the installed-versions dataset viaInstalledVersions::reload(), restoring the real data intearDown().Tests
test_reports_service_version_when_set— explicit version is emitted.test_omits_service_version_when_composer_reports_placeholder—1.0.0+no-version-setis dropped.test_omits_service_name_when_composer_reports_placeholder—__root__is dropped.