Navigation Menu

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

Collect which layer was used for invocation #1031

Merged
merged 1 commit into from Sep 20, 2021
Merged

Collect which layer was used for invocation #1031

merged 1 commit into from Sep 20, 2021

Conversation

deleugpn
Copy link
Member

This Pull Request introduces a new StatsD metric to analyse Bref usage among different layers. One open question before merging is whether we want to collect PHP Version in use. My initial idea is that we can follow Composer installation stats to see php version usage trend, which makes the layer the most important informmation to extract from Bref.

Since LambdaRuntime is an internal and final class, we won't consider this a breaking change.

@GrahamCampbell
Copy link
Contributor

👍 would be another good thing to include in the 1.3.0 release.

Copy link
Contributor

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

This is a good idea. Thank you!

@t-richard
Copy link
Member

Seems like a good idea but wouldn't it be simpler and more accurate to send the php version at the same time rather than relying on packagist and crossing data?

@mnapoli
Copy link
Member

mnapoli commented Sep 20, 2021

Awesome, this will help get a better vision of which runtimes are used the most.

This is helpful for Bref's own development, but also when talking with AWS.

For PHP versions, it's a "nice to have" but much less important than layers, because we can retrieve that information from somewhere else (https://packagist.org/packages/bref/bref/php-stats). I don't want to block that improvement.

Thanks Marco!

@mnapoli mnapoli merged commit efa0819 into master Sep 20, 2021
@mnapoli mnapoli deleted the statsd branch September 20, 2021 09:02
@chekalsky
Copy link

chekalsky commented Sep 20, 2021

@mnapoli this was non-BC change for those using php-runtime. But I see @Nyholm already fixing this. Can I hope for the soon release?

PHP Fatal error: Uncaught ArgumentCountError: Too few arguments to function Bref\Runtime\LambdaRuntime::fromEnvironmentVariable(), 0 passed in /var/task/vendor/runtime/bref/src/BrefRunner.php on line 27 and exactly 1 expected in /var/task/vendor/bref/bref/src/Runtime/LambdaRuntime.php:48

@deleugpn
Copy link
Member Author

@chekalsky LambdaRuntime is an internal class.

@Nyholm
Copy link
Contributor

Nyholm commented Sep 20, 2021

I made a release on runtime/bref just a few minutes ago.

@deleugpn is correct. The LambdaRuntime is internal. The runtime/bref package should either consider creating its own copy of LambdaRuntime or we should consider taking steps to make it non-internal.

@chekalsky
Copy link

chekalsky commented Sep 20, 2021

@deleugpn sorry, didn't meant that. Yes, would be great to mitigate possibility of this potential issue in the future.

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

Successfully merging this pull request may close these issues.

None yet

6 participants