Add new Slack handlers using Slackbot and webhooks #846

hkdobrev · 2016-09-06T21:00:03Z

This is keeping the previous SlackHandler, but extracts the message formatting logic and introduces two new Slack handlers:

SlackbotHandler - this is the simplest way to log to Slack and most people could use that. It doesn't allow fancy formatting, but has autolinking and the simplest configuration with two clicks in the Slack settings and getting a token.
SlackWebhookHandler - This is a bit more advanced as it allows for the same formatting and adding attachements as the SlackHandler, but it uses the Slack Webhooks which are quite easier to set up than the OAuth API. The minimum it requires is a webhook URL which is a retrievable with about two clicks in the Slack settings. Even the channel is optional as it could be configured in the integration.

This is probably not the leanest way to implement what we've discussed in #743, but after working at this I believe this is the simplest and the right approach.

While we could implement everything in one handler the API would become horrible and it would be harder to understand from users. Providing choice with simple to use separate classes is better IMHO. Apart from this the SlackHandler extends the SocketHandler and the webhooks and the Slackbot wouldn't work nicely with that.

What do you think?

@Seldaek @gkedzierski @Gummibeer @nisbeti

Extracting Slack formatting logic in SlackRecord
SlackbotHandler
SlackWebhookHandler
Refactor SlackHandler using the new SlackRecord
Add new handlers to the docs
Unit tests of SlackRecord
Unit tests for SlackbotHandler
Unit tests for SlackWebhookHandler
Update unit tests for SlackHandler

Gummibeer · 2016-09-06T21:15:36Z

I will Test it tomorrow and give you Feedback.

Seldaek · 2016-09-18T15:46:31Z

This seems fine to me implementation wise, if you can write tests for SlackRecord at least that'd be nice, testing the curl calls isn't really easy nor worth it IMO.

I'd also be happy to get feedback from people actually using this.. :)

Seldaek · 2016-09-25T15:55:20Z

BTW it might be worth looking at this at the same time if the Slack stuff gets a rewrite.. #745

And also marking the original SlackHandler as deprecated would be good I think (I will remove it from master/2.0 then), or do you think there is really a point in having 3?

greeny · 2016-09-26T09:36:19Z

I know that this might be a case for all handlers, not just this one, but what if I have disabled curl extension? There is also way to send requests through file_get_contents (you can check my implementation in https://github.com/greeny/nette-slack-logger/blob/master/src/SlackLogger.php#L77). Are we going to support this?

hkdobrev · 2016-09-26T09:40:32Z

@Seldaek I've started working on the tests and while doing so I've deprecated a couple of protected methods in SlackHandler which are better accessed through SlackRecord. However I don't see why we'd deprecate the SlackHandler. It's the only handler using the Slack API directly.

@greeny Could you point me to Monolog handlers which employ either cURL and if not available fall back to file_get_contents()? I don't think using only file_get_contents() is nice. I guess we could use Guzzle instead which would do the check and the fallback for us.

greeny · 2016-09-26T09:45:43Z

@hkdobrev I did not check monolog for handlers, but I think many of them are firing requests through curl. Guzzle is fine solution, I was just asking, if servers without curl can use this handler too.

[offtopic] Regarding my implementation, that was first only internal service which i published later.[/offtopic]

Seldaek · 2016-09-26T09:52:59Z

Most handlers use curl or raw sockets yes. I think that's fine to be honest, it's the first time anyone even asks about it in all these years. I wouldn't add a dependency on guzzle just for that.

greeny · 2016-09-26T09:59:14Z

@Seldaek it is not mine case (i have curl everywhere), but I know some hostings, that have curl disabled. Thats why I have file_get_contents in my implementation.

But I agree curl is very common nowdays.

@hkdobrev what about support for non-emoji icons? ("icon_url": "https://slack.com/img/icons/app-57.png")

stof · 2016-10-13T08:41:43Z

src/Monolog/Handler/Slack/SlackRecord.php

+                        $attachment['fields'][] = array(
+                            'title' => "Extra",
+                            'value' => $this->stringify($record['extra']),
+                            'short' => $this->useShortAttachment,


could be true here, as we know the value. It may be more readable

stof · 2016-10-13T08:42:02Z

src/Monolog/Handler/Slack/SlackRecord.php

+                            $attachment['fields'][] = array(
+                                'title' => $var,
+                                'value' => $val,
+                                'short' => $this->useShortAttachment,


could be false directly

stof · 2016-10-13T08:43:22Z

src/Monolog/Handler/SlackHandler.php

        if (!extension_loaded('openssl')) {
-            throw new MissingExtensionException('The OpenSSL PHP extension is required to use the SlackHandler');
+            throw new MissingExtensionException(
+                'The OpenSSL PHP extension is required to use the SlackHandler'


this change should be reverted

stof · 2016-10-13T08:43:34Z

src/Monolog/Handler/SlackHandler.php

+        $level = Logger::CRITICAL,
+        $bubble = true,
+        $useShortAttachment = false,
+        $includeContextAndExtra = false


Monolog always has the signature on one line

stof · 2016-10-13T08:45:08Z

src/Monolog/Handler/SlackbotHandler.php

+    private $channel;
+
+    /**
+     * @param  string $token   Slackbot token


missing phpdoc for $slackTeam

stof · 2016-10-13T08:45:27Z

src/Monolog/Handler/SlackWebhookHandler.php

+
+    /**
+     * @param  string      $webhookUrl             Slack Webhook URL
+     * @param  string      $channel                Slack channel (encoded ID or name)


string|null

martijncalker · 2016-11-04T10:41:40Z

src/Monolog/Handler/Slack/SlackRecord.php

+                }
+            }
+
+            $dataArray['attachments'] = json_encode(array($attachment));


Not sure if I use it the right way, but now I've got:

{"username":"TestBot","text":"","attachments":"[{\\"fallback\\":\\"Testing Slackhandler!\\",\\"color\\":\\"good\\",\\"fields\\":[{\\"title\\":\\"Level\\",\\"value\\":\\"INFO\\",\\"short\\":true}],\\"title\\":\\"Message\\",\\"text\\":\\"Testing Slackhandler!\\"}]","channel":"development-logging"}

Changing this line to:

$dataArray['attachments'] = [$attachment];

Output:

{"username":"TestBot","text":"","attachments":[{"fallback":"Testing Slackhandler!","color":"good","fields":[{"title":"Level","value":"INFO","short":true}],"title":"Message","text":"Testing Slackhandler!"}],"channel":"development-logging"}

@martijncalker Thanks for your feedback! This is now fixed thanks to the work of @an1zhegorodov

Seldaek · 2016-11-13T19:30:25Z

@hkdobrev any update here? Should I wait a couple days to get a chance to include this in the next 1.x release or will you need more time to finalize it? No stress I just need to know :)

mintobit · 2016-11-13T20:41:49Z

@Seldaek Wait couple of days pls. I'm using custom handler on my project, but would love this implementation. I've just finished tests for SlackRecord, planning to fix tests for SlackHandler and fix things related to @stof comments.

mintobit · 2016-11-13T22:42:40Z

@Seldaek done. Now waiting for feedback from @hkdobrev

hkdobrev · 2016-11-14T22:37:18Z

@an1zhegorodov Thank you for your work! SlackRecord wasn't really my code as I just extracted it from the previous SlackHandler and I wasn't really keen on refactoring it. I have now cherry-picked your commits and fixing conflicts in the meantime as I had some things locally which I haven't pushed. For example I've moved colours to constants, deprecated old methods and exposed the SlackRecord instance instead. I've started with a couple of tests and I've merged our SlackRecordTest.

Could you please confirm that your changes didn't incur any backwards incompatible changes? Not just to the PHP API, but also to the way the pre-existing SlackHandler is configured?

hkdobrev · 2016-11-14T22:41:37Z

src/Monolog/Handler/Slack/SlackRecord.php

-                    'value' => $record['level_name'],
-                    'short' => true,
-                );
+                $attachment['fields'][] = $this->generateAttachmentField('Level', $record['level_name'], true);


@an1zhegorodov I think here in the else of if ($this->useShortAttachment) we should really pass false for the short attachment option instead of true, don't you think? Just like @stof's comment: #846 (comment)

@stof While I agree with your comment and would have changed it without thinking twice, this is an example where not using the property introduces a bug more easily.

I'm sorry for commenting on an outdated diff, will copy my comment on the updated line as it is still valid and so the comment could be visible.

hkdobrev · 2016-11-14T22:43:41Z

src/Monolog/Handler/Slack/SlackRecord.php

+                $attachment['title'] = $record['level_name'];
+            } else {
+                $attachment['title'] = 'Message';
+                $attachment['fields'][] = $this->generateAttachmentField('Level', $record['level_name'], true);


@an1zhegorodov I think here in the else of if ($this->useShortAttachment) we should really pass false for the short attachment option instead of true, don't you think? Just like @stof's comment: #846 (comment)

@stof While I agree with your comment and would have changed it without thinking twice, this is an example where not using the property introduces a bug more easily.

@hkdobrev It's not about useShortAttachment in this case. Check out current implementation. It also has 'short' => true. That's because level_name is really short: INFO, DEBUG, ERROR.

@an1zhegorodov Thanks for pointing this out! I got confused from the if-else and for being away from this for a while.

hkdobrev · 2016-11-14T22:47:10Z

@greeny

@hkdobrev what about support for non-emoji icons? ("icon_url": "https://slack.com/img/icons/app-57.png")

This is a good suggestion. However, I wanted to limit the scope of this PR to provide the current functionality, but through the Slackbot or Webhooks without going through the Slack API. I think it should be extended to support everything Slack supports. But this could be done in a later Monolog release, ideally in 2.x when it could be refactored with breaking the public interface of the handler and its configuration so it could have a more flexible way to support everything Slack offers and even future features of Slack webhooks without changing the API and addition additional arguments to methods.

What do you think? Do you think this should be done as part of this PR? Cheers!

hkdobrev · 2016-11-14T22:50:05Z

@Seldaek

And also marking the original SlackHandler as deprecated would be good I think (I will remove it from master/2.0 then), or do you think there is really a point in having 3?

I've marked a couple of methods as deprecated now. However, I disagree with marking the whole handler as deprecated as people who already use the Slack API could have a valid case and could continue to use it. Using the Slack API is not deprecated, so I don't see why we should deprecated the handler. Although, 2.x would be a good place to refactor the public interface and allow for more flexibility so the interface doesn't depend on every property Slack supports, I don't see why to deprecate the whole method of logging to the Slack API.

What do you think? Are you still considering removing the whole handler in 2.x?

hkdobrev · 2016-11-14T23:01:21Z

Please let me know if you'd like commits squashed or partially squashed.

Thanks to all for all the feedback and help! I'm sorry for delaying it!

mintobit · 2016-11-14T23:53:17Z

Could you please confirm that your changes didn't incur any backwards incompatible changes? Not just to the PHP API, but also to the way the pre-existing SlackHandler is configured?

@hkdobrev I didn't change public API, only made SlackHandler constructor one line. Any hints what I should look for? I think it's worth to try it out locally on 1.x, what do you think?

mintobit · 2016-11-15T00:39:43Z

src/Monolog/Handler/Slack/SlackRecord.php

+                }
+            }
+
+            $dataArray['attachments'] = array($attachment);


Removed json_encode here. I think this will break SlackHandler, despite working fine with SlackWebhookHandler. SlackHandler sends data with application/x-www-form-urlencoded headers, and it looks like slack expects attachments field to be json_encoded in that case.

A few optoins I figured:

json_encode attachments in SlackHandler

change content-type to application/json and json_encode the whole record in SlackHandler

I like second more. What do you think @Seldaek @hkdobrev ?

@an1zhegorodov Could you elaborate more on the second option? Would that work with the Slack API?

@hkdobrev That should work according to Slack API documentation

invalid_post_type
The method was called via a POST request, but the specified Content-Type was invalid. Valid types are: application/json application/x-www-form-urlencoded multipart/form-data text/plain.

Using json will also make SlackHandler tests more clean and simple. No regex required.

I'll work on this tonight and test it out. Sorry for missing that in the first place.

I was wrong. Slack API docs are confusing, they actually don't support json.

@an1zhegorodov That may be an outdated answer, though.

It's from 2015 (There's a comment from this month saying it doesn't support it 😞 )

The Slack API documentation you have linked earlier says JSON is supported.

Slack API website now has a message builder tool and it uses JSON exclusively. See an example.

Have you tried it out on the API? I might be able to try it out tomorrow if you won't be able to.

@hkdobrev

It's still true, I tested it via Postman and I have the same error message they had in 2015

It says that application/json is a valid value for Content-Type header. Well, it doesn't complain about that header value indeed 😆

The comment you mentioned says exactly that. I think they use JSON there just for the sake of editing simplicity, it's not processed by an API, but by custom controller.

Never mind. Finishing it.

mintobit · 2016-11-16T00:13:02Z

src/Monolog/Handler/SlackWebhookHandler.php

+     * @param  int         $level                  The minimum logging level at which this handler will be triggered
+     * @param  bool        $bubble                 Whether the messages that are handled can bubble up the stack or not
+     */
+    public function __construct(


@hkdobrev Are we going to one-line arguments for consistency?
Also, we can make the order of arguments the same as in SlackHandler (except here we'll have webhookUrl instead of token). This makes switching between SlackHandler and SlackWebhookHandler much easier.

@an1zhegorodov Yes, I'd change __construct() to be on a single line everywhere as per @stof's comment. I was going more for PSR-2 conventions.

However, I wouldn't want to unify argument order as it used to be in SlackHandler. See #197. I think the order in SlackHandler is the old order which would be changed in 2.0. So it would be easier to switch to 2.0 if you use the new Slack handlers, the old one is like that anyway so if you SlackHandler you'd need to change it for 2.0 anyway.

So anyone is welcome to suggest a more suitable order of arguments for SlackWebhookHandler and SlackbotHandler to adhere more to 2.0 styleguide, but I wouldn't want to make them like SlackHandler.

mintobit · 2016-11-16T00:21:26Z

src/Monolog/Handler/SlackHandler.php

+            E_USER_DEPRECATED
+        );
+
+        return $this->slackRecord->getAttachmentColor($level);


Just a thought. What if someone extended SlackHanlder just to make colors mapping different? He'll get default colors mapping after update, right? Looks like we're breaking backwards compatibility here. Same happens with stringify.

Yes. We would be breaking extensions which adhere to the Liskov principle, because we are not calling this method anymore. The deprecation notice would help, but not to people not familiar with Monolog. However, this is only about people extending SlackHandler as we are not changing anything for people using the public API. This is only the protected API. So I think we could close our eyes for that BC change here, because we have a deprecation notice for people who extended it.

@Seldaek Do you think it's OK breaking the use of classes extended SlackHandler?

Yeah I think I can live with a minor break there.

Seldaek · 2016-11-16T15:29:10Z

Note that the 2.0 constructor arguments are still very likely to change so don't attach too much importance to that.. Make it work for 1.x and we'll see.

mintobit · 2016-11-16T16:28:01Z

@Seldaek Done that yesterday. @hkdobrev needs some time to try it out.

I'm using SlackHandler at work and have some improvements ideas, but I guess that'll be a separate PR.

mirfilip · 2016-11-18T13:16:38Z

I haven't looked into the code yet, but I have a question. I'm currently using this SlackWebhookHandler. The one thing which is missing is the good formatter for $context and $extra. Yes, it's readable for the main text/message, but when you pass context/extra, there is no good formatter available.

The formatter that would pretty print the array would be preferred. Back when my colleagues and I used our own logger, we used to turn on the markdown option (mrkdwn) and add an attachment with json serialized text. That's the prettiest way we come up with.

Any solutions to this problem here?

mintobit · 2016-11-18T14:18:55Z

@mirfilip There's a chance that your issues are solved here, I have added stringify to those extra/context fields which are arrays.

Here's how it looks with those changes I mentioned.

Conflicts: - tests/Monolog/Handler/Slack/SlackRecordTest.php - merged the tests

…level deep

… construction

Slack API requires to json_encode the attachment field.

This is for consistency with the rest of the handlers.

mirfilip · 2016-11-21T09:37:32Z

@an1zhegorodov Stringify is indeed some improvement, but with multidimensional arrays it's still pretty much useless.

One way I've seen this solved is either:

Normalize (NormalizerFormatter) first, then json encode with pretty print (or any serialize with pretty printing)
or
print_r or similar

mintobit · 2016-11-21T14:16:33Z

@mirfilip Once this is merged, I'll come up with a separate PR addressing this and a few other issues. Would appreciate your feedback.

hkdobrev · 2016-11-21T21:22:17Z

@mirfilip @an1zhegorodov I also think it's better a new issue to be opened for this and tackled separately from the majority of the Slack handlers work.

wysow · 2016-11-24T11:44:30Z

Hey guys, Thanks for this amazing work, can we see it merged soon @Seldaek ?

Seldaek · 2016-11-25T23:40:13Z

Thanks everyone :)

Seldaek · 2016-11-26T00:25:04Z

1.22 is out..

- [x] Exclude `extra`/`context`, `datetime`, `level` from message when attachment is used - [x] Use `ts` attachment key to display `datetime` considering user timezone - [x] [Support](#846 (comment)) custom user images - [x] [Allow](#894 (comment)) to setup username from slack - [x] [Improve](#846 (comment)) array formatting within `context`/`extra` - [x] [Support](#745) `include_stacktraces` option when attachment is not used and always include stacktraces when attachment is used - [x] Support `extra`/`context` field exclusion - [x] Update tests

gmponos · 2018-03-15T20:33:02Z

May I ask why cURL was preferred over Socket? Is it possible you could to move to socket instead of cURL?

hkdobrev · 2018-03-15T20:41:21Z

The SlackHandler uses a socket with the streaming API. The simpler SlackbotHandler and the SlackWebhookHandler use cURL.

I'm not sure if these handlers could use a socket. cURL is simpler and based on their examples in the docs. I think only the streaming API would require a socket.

Why do you need to use a socket? If you use webhooks or the Slackbot, their examples give you just a webhook URL.

gmponos · 2018-03-15T21:06:47Z

I hope I am not saying something incredibly stupid but I think using Sockets you can fire and forget an HTTP request from PHP.

https://stackoverflow.com/a/14668762/4158811
https://stackoverflow.com/a/14982587/4158811

The reason is that I don't want my application to break/stay halt if my Slack service is down.

I can see also that with the current implementation of SlackWebhook it retries the cURL request five times and I don't see a timeout setting so most probably my app will stay frozen while trying to call the webhook.

Not sure though if SocketHandler of monolog is also waiting for the response or it has a "fire and forget" logic.

mintobit · 2018-03-16T12:16:51Z

"Fire and forget" would only work if there's no need to write data to the socket. We need to do that, so it would be locking operation with either Sockets or cURL.
To avoid application crash if Slack is down, you can use WhatFailureGroupHandler.
Timeout setting is indeed missing, can be easily implemented the same way as in SocketHandler with fallback to ini settings.

If performance is critical, I'd also consider using AmqpHandler or RedisHandler and then sending messages to Slack using separate process.

Seldaek mentioned this pull request Sep 18, 2016

Getting token to use Monolog with Slack #743

Closed

Seldaek added this to the 1.x milestone Sep 18, 2016

Seldaek added the Feature label Sep 18, 2016

Seldaek mentioned this pull request Sep 25, 2016

SlackHandler: do not send username and icon #849

Closed

stof reviewed Oct 13, 2016

View reviewed changes

martijncalker reviewed Nov 4, 2016

View reviewed changes

hkdobrev commented Nov 14, 2016

View reviewed changes

mintobit reviewed Nov 15, 2016

View reviewed changes

mintobit reviewed Nov 16, 2016

View reviewed changes

hkdobrev and others added 15 commits November 19, 2016 18:53

Add initial tests for SlackRecord

7c2f58e

No need for extra json_encode

97eb782

Add more tests to SlackRecordTest

ec75076

Conflicts: - tests/Monolog/Handler/Slack/SlackRecordTest.php - merged the tests

Make sure extra/context variables are stringified when more than one …

4ab8ed0

…level deep

Codereview fixes based on @stof comments

01a2ac2

Update SlackHandler tests && allow SlackRecord formatter change after…

08b577c

… construction

Refactor SlackRecord

1303dc6

Fix SlackRecordTest

d34de6b

Add missing PHPDoc types

dd23889

Fix SlackHandler

5fa5e37

Slack API requires to json_encode the attachment field.

Make message formatting behaviour same as previous version

506e1b9

Format constructors on a single line

8b2b135

This is for consistency with the rest of the handlers.

Do not put empty Slack attachments array by default

26b526d

Add unit tests for SlackWebhookHandler

43e1e69

Add unit test for SlackbotHandler

9fbc286

Seldaek merged commit e94929e into Seldaek:1.x Nov 25, 2016

mintobit mentioned this pull request Nov 27, 2016

Slack improvements #894

Merged

8 tasks

jewome62 mentioned this pull request Nov 29, 2016

Add SlackWebhookHandler and SlackbotHandler into configuration symfony/monolog-bundle#191

Merged

hkdobrev deleted the slack-webhooks branch March 15, 2018 20:38

Add new Slack handlers using Slackbot and webhooks #846

Add new Slack handlers using Slackbot and webhooks #846

Conversation

hkdobrev commented Sep 6, 2016 • edited Loading

Gummibeer commented Sep 6, 2016

Seldaek commented Sep 18, 2016

Seldaek commented Sep 25, 2016

greeny commented Sep 26, 2016

hkdobrev commented Sep 26, 2016 • edited Loading

greeny commented Sep 26, 2016 • edited Loading

Seldaek commented Sep 26, 2016

greeny commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seldaek commented Nov 13, 2016

mintobit commented Nov 13, 2016 • edited Loading

mintobit commented Nov 13, 2016

hkdobrev commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkdobrev commented Nov 14, 2016

hkdobrev commented Nov 14, 2016

hkdobrev commented Nov 14, 2016

mintobit commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mintobit Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkdobrev Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seldaek commented Nov 16, 2016 via email

mintobit commented Nov 16, 2016

mirfilip commented Nov 18, 2016 • edited Loading

mintobit commented Nov 18, 2016 • edited Loading

mirfilip commented Nov 21, 2016 • edited Loading

mintobit commented Nov 21, 2016

hkdobrev commented Nov 21, 2016

wysow commented Nov 24, 2016

Seldaek commented Nov 25, 2016

Seldaek commented Nov 26, 2016

gmponos commented Mar 15, 2018

hkdobrev commented Mar 15, 2018

gmponos commented Mar 15, 2018 • edited Loading

mintobit commented Mar 16, 2018

hkdobrev commented Sep 6, 2016 •

edited

Loading

hkdobrev commented Sep 26, 2016 •

edited

Loading

greeny commented Sep 26, 2016 •

edited

Loading

mintobit commented Nov 13, 2016 •

edited

Loading

mintobit Nov 15, 2016 •

edited

Loading

hkdobrev Nov 15, 2016 •

edited

Loading

mirfilip commented Nov 18, 2016 •

edited

Loading

mintobit commented Nov 18, 2016 •

edited

Loading

mirfilip commented Nov 21, 2016 •

edited

Loading

gmponos commented Mar 15, 2018 •

edited

Loading