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

Improve test-command and developer-experience #343

Open
cklm opened this issue May 8, 2020 · 8 comments
Open

Improve test-command and developer-experience #343

cklm opened this issue May 8, 2020 · 8 comments

Comments

@cklm
Copy link
Contributor

cklm commented May 8, 2020

Hi, we just stumbled across a wrong message in the sentry:test command. As it turned out, the php-configuration on that server was missing the cacert.pem-file containing root-certificates to verify certificates sent by webservers.

However, the command told us

$ php bin/console sentry:test -vvv
DSN correctly configured in the current client
Sending test message...
Message sent successfully with ID xxx

Thats not correct - there was no message sent, since curl fails in the background with SSL certificate error: unable to get local issuer certificate. So we searched for hours in the firewall and outbound-connection assuming the problem is there somewhere, since sentry told that the message was successully sent.

That should be changed, that the command really tests setup and connection and not only the attempt to send a message (or it should be clarified in the message).

Thanks!

@Jean85
Copy link
Collaborator

Jean85 commented May 8, 2020

The behavior of the HTTP layer is unfortunately out of our control. Maybe switching to Guzzle would make those errors bubble up?

BTW these errors would surface better with getsentry/sentry-php#989, which is slated to be released as 2.4.

@krewetka
Copy link

krewetka commented Oct 23, 2020

I have the same issue. sentry:test command is not really usefull when there is delayed sending at the end of processing.

It shows message was sucessully sent when it was just added to the queue.

If there is such debug command - it should not use delay dispatching. As this is not showing real problems like ssl certificate problems.

@Jean85 it is not about HTTP layer behaviour.

It is about HttpTransport.php in Sentry which is not really even trying to send message when there is delay setting:

image

Problem is this is also used for test command which just add message to pendingRequests without even trying to send it - but shows it was successfully send!

Took me hours to debug that in reality I have SSL peer certificate or SSH remote key was not OK error and test command was showing everything is fine.

@Jean85
Copy link
Collaborator

Jean85 commented Oct 26, 2020

Ouch, I forgot about the delayed sending. In the next major version that will be no longer the default behaviour, but we need to fix it here. Maybe we can use flush() to force it to send the event?

@Jean85
Copy link
Collaborator

Jean85 commented Jan 19, 2021

@ste93cry is this still needed with 4.0?

@ste93cry
Copy link
Collaborator

The client still returns null from the capture*() methods without giving any hint about why it failed to send the event. We now support a PSR-3 logger instance to debug such things, but it may still be worth to improve the command's UX. However, I tried to do that for the 4.0 milestone and I didn't find a good way nor I have an idea of what we could do to improve the situation as my opinion is that the command is not really useful because sending of an event through the client does not guarantee that all the things that would act on a normal "sending flow" are executed, so if the purpose of this command is to aid debugging then it's not really reliable

@github-actions
Copy link

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@ste93cry
Copy link
Collaborator

I'm putting this on the backlog because I know that the DX on Laravel is far better than our, so it makes sense for me to keep track of this issue and take some time in the future to make the same improvements here

@Jean85
Copy link
Collaborator

Jean85 commented Jan 12, 2022

#372 seems related to this.

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

No branches or pull requests

5 participants