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

Upgrade to Blackfire agent v2 in web container #2970

Merged
merged 9 commits into from
May 3, 2021

Conversation

thomasdiluccio
Copy link
Contributor

@thomasdiluccio thomasdiluccio commented Apr 28, 2021

DDEV-Local implements Blackfire agent V1 which is now deprecated and will be soon unsupported. This PR bootstrap the upgrade to Blackfire agent V2.

I haven't been able to test this code and look forward to some procedures to do so.

@rfay
Copy link
Member

rfay commented Apr 28, 2021

Thanks fo rthis!

You can test this manually either on your own machine or using https://gitpod.io/#https://github.com/drud/ddev/pull/2970

  1. Build the web container with a tag, cd containers/ddev-webserver && make VERSION=blackfire-agent-v2
  2. Update the tag in pkg/version/version.go
  3. make in project root

If you do it on gitpod, the built ddev binary is already in the PATH and should "just work". If you do it locally, you have to path to the built ddev.

There's a bit more detail in https://github.com/drud/ddev/blob/master/docs/developers/building-contributing.md

Obviously a real image has to be pushed up to dockerhub when you're ready, so when it's tested just let me know when you need that pushed.

@thomasdiluccio
Copy link
Contributor Author

Thank you @rfay! I will follow you instructions and keep you posted once the PR should be good to be reviewed.

@gitpod-io
Copy link

gitpod-io bot commented May 2, 2021

@rfay
Copy link
Member

rfay commented May 2, 2021

I pushed the webimage for this and rebased this (so before you work on it more, please make sure to reset to the upstream).

@thomasdiluccio
Copy link
Contributor Author

@rfay I've been able to test and update the PR in Gitpod. It should be good to go now I've been able to activate backfire, successfully perform a profile on the phpinfo() page, and deactivate it.

All 3 commands (ddev blackfire on, ddev blackfire off, and ddev blackfire status) are working as expected.

Thanks for your help

@rfay
Copy link
Member

rfay commented May 3, 2021

Thanks, I'll give it a try today!

@rfay rfay changed the title Bootstrap the upgrade toward Blackfire agent v2 Upgrade to Blackfire agent v2 in web container May 3, 2021
Copy link
Member

@rfay rfay 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 looking great, thanks! I added 79fd560 to replace obsolete usages that were looking for blackfire-agent. Would love if you could take a final look at it. Thanks so much for pursuing this.

agentstatus=$?
# Can't use killall here because it kills this process!
agentstatus=$(ps -ef | awk '$8~/^blackfire.*/ { print $2 }' 2>/dev/null)
if [ "${pid}" != "" ]; then kill $pid; fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why killing the process here? killall was used in the first place, passing -0 to check if it is running, right? AFAICS, the same thing is done thanks to ps -ef.
IMHO, the kill command should be removed. Otherwise checking the status would also stop the agent, which seems to be counter-productive 🤔

Copy link
Member

Choose a reason for hiding this comment

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

killall kills this process, which is named "blackfire", so that's why I had to change it.

Pushed another commit to remove the copy-pasta error that kills the pid in ddev blackfire status. Good eye, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool thanks 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rfay and @lolautruche. It seems we are good to go! I love when a plan comes together ;)

Copy link
Member

Choose a reason for hiding this comment

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

Sure glad you fixed this up. I hadn't even become aware of the new blackfire version and deprecation.

@rfay
Copy link
Member

rfay commented May 3, 2021

Thanks all!

@rfay rfay merged commit 281070c into ddev:master May 3, 2021
@rfay rfay mentioned this pull request May 6, 2021
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.

None yet

3 participants