Skip to content
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.

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

Log invalid log levels, then default to 'debug' #216

Merged
merged 1 commit into from
Jun 7, 2021

Conversation

pjohnmeyer
Copy link
Contributor

Prior to this change, an invalid log level passed to the FilteredLogLevelDecorator
constructor would result in an InvalidArgumentException. This means that a mis-
configured production system could fail to boot.

This patch proposes that an invalid log level should not be a hard failure. The
change logs the invalid log level to the 'realLogger' as an error, then sets the
default log level to 'debug'.

Prior to this change, an invalid log level passed to the FilteredLogLevelDecorator
constructor would result in an InvalidArgumentException. This means that a mis-
configured production system could fail to boot.

This patch proposes that an invalid log level should not be a hard failure. The
change logs the invalid log level to the 'realLogger' as an error, then sets the
default log level to 'debug'.
@asgrim asgrim self-assigned this Jun 5, 2021
@asgrim asgrim added the enhancement New feature or request label Jun 5, 2021
@pjohnmeyer
Copy link
Contributor Author

pjohnmeyer commented Jun 5, 2021

@asgrim not clear to me why the Psalm checks failed; they don't fail on my dev machine, and they didn't fail in GitHub Actions on the files that I changed. I'll take a closer look at the Unit test failures in the next few days.

@asgrim
Copy link
Collaborator

asgrim commented Jun 5, 2021

I'll check next week too if not :)

@pjohnmeyer
Copy link
Contributor Author

pjohnmeyer commented Jun 6, 2021

@asgrim hope this helps:

Summary

Quality Checks

The Psalm checks are behaving differently in CI than locally, and are only failing on files that are not modified by this PR. This is a separate issue and I recommend overriding this check and fixing it under a separate PR.

Main Tests

These seem to be failing because of a missing secret, secrets.SCOUT_APM_KEY. This is because repo secrets aren’t available to fork PRs. The immediate solution would be to recreate this PR from a branch in your repo.

Details

Psalm

When I was running PHP 8.0.3 on my development machine I received no errors. The CI pipeline is currently using PHP 8.0.7, so I updated my PHP and I got six errors:

ERROR: RedundantCastGivenDocblockType - src/Events/Metadata.php:162:51 - Redundant cast to string given docblock-provided type (see https://psalm.dev/263)
                        $packageName === 'root' ? (string) InstalledVersions::getRootPackage()['name'] : $packageName,


ERROR: PossiblyNullArgument - src/Events/Metadata.php:165:29 - Argument 2 of sprintf cannot be null, possibly null value provided (see https://psalm.dev/078)
                            InstalledVersions::getPrettyVersion($packageName),


ERROR: PossiblyNullArgument - src/Events/Metadata.php:166:29 - Argument 3 of sprintf cannot be null, possibly null value provided (see https://psalm.dev/078)
                            InstalledVersions::getReference($packageName)


ERROR: RedundantCastGivenDocblockType - src/Helper/ComposerPackagesCheck.php:51:16 - Redundant cast to bool given docblock-provided type (see https://psalm.dev/263)
        return (bool) InstalledVersions::isInstalled($package);


ERROR: PossiblyNullArgument - tests/Unit/Events/MetadataTest.php:67:21 - Argument 2 of sprintf cannot be null, possibly null value provided (see https://psalm.dev/078)
                    InstalledVersions::getPrettyVersion($packageName),


ERROR: PossiblyNullArgument - tests/Unit/Events/MetadataTest.php:68:21 - Argument 3 of sprintf cannot be null, possibly null value provided (see https://psalm.dev/078)
                    InstalledVersions::getReference($packageName)

Again, none of these are in files I have modified. This is also a far cry from the 81 errors in CI. Out of curiosity, I used nektos/act to run the quality checks locally and I did get the same results that show up in GitHub Actions, but I did not consider investigating why.

@asgrim
Copy link
Collaborator

asgrim commented Jun 7, 2021

The static analysis issues come from Composer deciding to add type annotations somewhere between Composer 2.0.8 and 2.1.1 - previously there were not types defined, so I consumed the interface accordingly (cast to a type I expected). Now types are defined correctly in Composer (I didn't check exactly since when), we got redundant type conversions, and also one method I expected string but actually returns string|null etc. - as we should ideally support across as many versions as possible I've added a stub to fix up these issues in Psalm using the type definitions from the latest Composer - have made a new PR #217 to work on this.

Thank you so much @pjohnmeyer - the change itself was 👌 just an unexpected dependency issue slipped in :)

As for secrets not being available in PRs, that is indeed an issue. I'll have to look into how we can improve that in future (created #218).

@asgrim asgrim merged commit abb1e3a into scoutapp:master Jun 7, 2021
asgrim added a commit that referenced this pull request Jun 7, 2021
@pjohnmeyer
Copy link
Contributor Author

Thanks for doing the remaining legwork to get this incorporated, @asgrim -- much appreciated. We will look forward to the next package release!

@asgrim
Copy link
Collaborator

asgrim commented Jun 9, 2021

@pjohnmeyer I've just released 6.2.0 with this fix in, thank you! https://github.com/scoutapp/scout-apm-php/releases/tag/v6.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants