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

feat(collectors): add command and scheduled tasks collectors #76

Merged

Conversation

countless-integers
Copy link
Collaborator

@countless-integers countless-integers commented Sep 21, 2020

Adds collectors for cli commands and scheduled tasks.

Closes #75

todo:

  • tests 1
  • extra metadata (especially for scheduled tasks) (?)

1: not sure what to do with tests for the scheduler the classes are missing in L5.5 + some issues with starting test in command collector

@countless-integers countless-integers self-assigned this Sep 21, 2020
@countless-integers countless-integers changed the title feat(collectors): add command and scheduled tasks collectors feat(collectors): add command and scheduled tasks collectors [WiP] Sep 21, 2020
@countless-integers countless-integers marked this pull request as draft September 21, 2020 09:27
@countless-integers countless-integers changed the title feat(collectors): add command and scheduled tasks collectors [WiP] feat(collectors): add command and scheduled tasks collectors Sep 21, 2020
src/Agent.php Outdated Show resolved Hide resolved
@countless-integers countless-integers force-pushed the feat/add-command-and-scheduled-task-collectors branch 4 times, most recently from fc1d28b to 6d3cd66 Compare October 1, 2020 12:39
@countless-integers
Copy link
Collaborator Author

countless-integers commented Oct 1, 2020

It gets interesting when a command fails though. I'm missing some way to report the error :/ . Perhaps some global exception listener in the framework collector (if it's not there already and I'm just no seeing it, I have a vague recollection that I've seen errors reported before...)

Screenshot 2020-10-01 at 14 51 46

@arkaitzgarro
Copy link
Owner

It gets interesting when a command fails though. I'm missing some way to report the error :/ . Perhaps some global exception listener in the framework collector (if it's not there already and I'm just no seeing it, I have a vague recollection that I've seen errors reported before...)

CommandFinished event includes a $exitCode property. Can it be that the event is fired for both successful and failed commands? https://github.com/laravel/framework/blob/43bea00fd27c76c01fd009e46725a54885f4d2a5/src/Illuminate/Console/Application.php#L93

@countless-integers
Copy link
Collaborator Author

It gets interesting when a command fails though. I'm missing some way to report the error :/ . Perhaps some global exception listener in the framework collector (if it's not there already and I'm just no seeing it, I have a vague recollection that I've seen errors reported before...)

CommandFinished event includes a $exitCode property. Can it be that the event is fired for both successful and failed commands? https://github.com/laravel/framework/blob/43bea00fd27c76c01fd009e46725a54885f4d2a5/src/Illuminate/Console/Application.php#L93

I'd expect that. I'll see if I can reproduce it and adjust the "fake status code" from 200 to something else. If I remember correctly, in Symfony, the command could return a status code in the handle method and that was it.

However what I meant was when an exception is thrown and not handled in the command, then listener doesn't seem to fire, but the app terminate middleware executes send on the agent. In phikra there was this naughty thing that adding errors.

@countless-integers countless-integers marked this pull request as ready for review October 7, 2020 08:04
@countless-integers
Copy link
Collaborator Author

Ok, I've added the status code check.

I've also noticed I've been wrong about CommandFinished -- it doesn't seem to fire when there's an unhandled exception thrown in the command. Terminate middleware still sends data to APM though.

@countless-integers countless-integers force-pushed the feat/add-command-and-scheduled-task-collectors branch from 7f07546 to e41e343 Compare October 13, 2020 08:09
chore(ci): appease cs checker
Co-authored-by: Arkaitz Garro <arkaitz.garro@gmail.com>
@countless-integers countless-integers force-pushed the feat/add-command-and-scheduled-task-collectors branch from ab26a15 to 5de527c Compare October 13, 2020 13:48
@countless-integers countless-integers merged commit f59ac6b into master Oct 13, 2020
@countless-integers countless-integers deleted the feat/add-command-and-scheduled-task-collectors branch October 13, 2020 14:26
This was referenced Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a collector for artisan commands
2 participants