fix: code-level cleanup surfaced during psalm/phan upgrade work - #613
fix: code-level cleanup surfaced during psalm/phan upgrade work#613nickmarden wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #613 +/- ##
============================================
+ Coverage 81.69% 82.12% +0.43%
+ Complexity 1821 1689 -132
============================================
Files 129 108 -21
Lines 7144 6211 -933
============================================
- Hits 5836 5101 -735
+ Misses 1308 1110 -198 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 45 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
ChrisLightfootWild
left a comment
ChrisLightfootWild
left a comment
There was a problem hiding this comment.
@nickmarden thanks for picking some of these changes out from your previous PR.
Happy to move the psalm dependencies forward etc if you can get the working combination that seemed to elude renovate-bot. 😬
| } | ||
|
|
||
| return ResourceInfo::create(Attributes::create($attributes), Version::VERSION_1_38_0->url()); | ||
| return ResourceInfo::create(Attributes::create($attributes), ResourceAttributes::SCHEMA_URL); |
There was a problem hiding this comment.
Prior to this change, the instrumentation claims to emit telemetry adhering to 1.38.0 of the semantic conventions.
Changing to the deprecated ResourceAttributes class downgrades that claim to 1.32.0 (and then it may not be correct for that version).
I think this should be reverted.
There was a problem hiding this comment.
Agreed, and reverted. The detector keeps Version::VERSION_1_38_0->url().
For context on why I touched it at all: ContainerTest::test_valid_v1 asserts ResourceAttributes::SCHEMA_URL, so it disagrees with the detector and fails on main today, independently of this PR. I had fixed the mismatch from the wrong end. It now fixes the test instead, in a separate commit so it is easy to review on its own.
- Remove deprecated InstrumentationInterface/InstrumentationTrait from AwsSdkInstrumentation. The interface methods were already implemented by the class itself; the two properties the trait contributed are now declared explicitly, and a constructor keeps the Noop defaults the trait's constructor used to install so behaviour is unchanged. - Add @phan-suppress-next-line for TransportInterface generic type narrowing in AwsLambdaWrapper OtlpExporter construction - PDO: fully qualify the `@var \Pdo\Sqlite` annotation. The `use PDO;` import made the relative `Pdo\Sqlite` resolve to `PDO\Sqlite` (class aliases are case-insensitive), which PHPStan reported as a class referenced with incorrect case on 8.2, 8.3 and 8.4. - PDO: drop the Sqlite ignore patterns. PHPStan ships PHP 8.4 stubs whatever version it runs on, so Pdo\Sqlite and its methods always resolve and the patterns went unmatched. Only PDO::connect() still needs ignoring below 8.4.
ContainerTest asserted ResourceAttributes::SCHEMA_URL while the detector emits Version::VERSION_1_38_0->url(), so the test failed on main. An earlier revision of this PR resolved the mismatch by changing the detector to ResourceAttributes::SCHEMA_URL. As pointed out in review, that silently downgrades the detector's semantic conventions claim from 1.38.0 to 1.32.0 (via the deprecated ResourceAttributes class), and the attributes may not even be correct for 1.32.0. The detector is left alone and the test is corrected instead. The package already requires open-telemetry/sem-conv ^1.38, so Version::VERSION_1_38_0 is guaranteed to be available.
f62ab5c to
f05606e
Compare
|
@ChrisLightfootWild lemme know if you need anything more on this PR, thanks. |
2 participants
Summary
Small set of correctness fixes surfaced while investigating CI failures from the psalm/phan upgrade work (PR #579, now superseded by the revert in #600). These are clean code-level changes with no suppressions beyond one narrow phan annotation.
Each of the three projects touched here is currently failing on
main, and these are the fixes for those failures.Aws— phan, all PHP versionsmainreports:AwsSdkInstrumentationalready implemented every interface method itself, so the interface and trait are dropped. The two properties the trait contributed ($propagator,$tracerProvider) are now declared explicitly, and a constructor installs the sameNoopTextMapPropagator/NoopTracerProviderdefaults that the trait's constructor used to set, so an instance stays usable beforesetPropagator()/setTracerProvider()are called.Also adds
@phan-suppress-next-line PhanTypeMismatchArgumentfor theTransportInterfacegeneric type narrowing onOtlpExporterconstruction inAwsLambdaWrapper.Instrumentation/PDO— phpstan, PHP 8.2 / 8.3 / 8.4mainreports the same error on every version:The cause is the
use PDO;import inPDOInstrumentationTest: class aliases are case-insensitive, so the relative@var Pdo\Sqliteresolves through it toPDO\Sqlite. Fully qualifying it as@var \Pdo\Sqlitefixes it.With that resolved, the
Pdo\Sqliteignore patterns are all obsolete — PHPStan ships PHP 8.4 stubs whatever version it runs on, so the class and its methods always resolve and the patterns went unmatched (which is itself an error underreportUnmatchedIgnoredErrors). OnlyPDO::connect()still needs ignoring below 8.4.ResourceDetectors/Container— phpunit, all PHP versionsContainerTest::test_valid_v1assertsResourceAttributes::SCHEMA_URLwhile the detector emitsVersion::VERSION_1_38_0->url(), so it fails onmain.An earlier revision of this PR resolved the mismatch by changing the detector. As @ChrisLightfootWild pointed out in review, that silently downgrades the detector's semantic conventions claim from 1.38.0 to 1.32.0 via the deprecated
ResourceAttributesclass. The detector is now left alone and the test is corrected instead, in its own commit. The package already requiresopen-telemetry/sem-conv: ^1.38.Test plan
Full
make all-checks(style, composer validate, phan, psalm, phpstan, phpunit) run locally in the project's Docker images:AwsResourceDetectors/ContainerInstrumentation/PDOOn 8.4 the
Pdo\Sqlitesubclass test runs for real rather than skipping.The remaining red jobs in this PR's matrix (
Symfony,Yii,Psr3,Monolog,Sampler/RuleBased,Laravel,ReactPHP,HttpConfig,CodeIgniter,ExtRdKafka) are pre-existing failures onmainand are untouched here.