Skip to content

Slack improvements #894

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

Merged
merged 8 commits into from
Dec 13, 2016
Merged

Slack improvements #894

merged 8 commits into from
Dec 13, 2016

Conversation

mintobit
Copy link
Contributor

@mintobit mintobit commented Nov 27, 2016

While #846 was focused on providing the ability to communicate with Slack via webhooks/bot, there's a bunch of improvements that can be done before 2.x. Thanks to @greeny, @websirnik, @jewome62 and @mirfilip for ideas.

  • Exclude extra/context, datetime, level from message when attachment is used
  • Use ts attachment key to display datetime considering user timezone
  • Support custom user images
  • Allow to setup username from slack
  • Improve array formatting within context/extra
  • Support include_stacktraces option when attachment is not used and always include stacktraces when attachment is used
  • Support extra/context field exclusion
  • Update tests

@Seldaek Seldaek added this to the 1.x milestone Nov 28, 2016
@jewome62
Copy link

Can you remove default value for username into SlackWebhookHandler (in construct)
We can into Slack for webhook define default name.
But with monolog, the default name is never use because we send Monolog by default.

Thanks

@mintobit
Copy link
Contributor Author

@jewome62 Good idea. This will allow to use slack internal username settings. I'll check how it works for slack API and get that done. Thanks.

@danizord
Copy link
Contributor

danizord commented Dec 1, 2016

Looking forward to the removal of context fields duplicated in message 👍

@mintobit
Copy link
Contributor Author

mintobit commented Dec 7, 2016

@Seldaek Do you think we can do better to support include_stacktraces?
#846 implemented getFormatter, thus if handler has a formatter which supports include_stacktraces and useAttachment === false - it works
In case useAttachment === true, we now have NormalizerFormatter in SlackRecord::stringify which will always include stacktraces and will ignore include_stacktraces. This works for people who want traces in slack, but doesn't work for those who don't.

The only way I see to fully support include_stacktraces is SlackFormatter, which would handle both message and attachment and would have includeStacktraces() setter.

@Seldaek
Copy link
Owner

Seldaek commented Dec 8, 2016

@an1zhegorodov seems like you say a way to do it so I'd say do it if you think there is value in it :)

@mintobit
Copy link
Contributor Author

mintobit commented Dec 8, 2016

@Seldaek Just realized it would also require moving most of constructor arguments to the formatter... We might consider that for 2.x

@mintobit
Copy link
Contributor Author

mintobit commented Dec 9, 2016

Some visuals on these changes:

@hkdobrev, @stof, would appreciate a code review

@riccardomessineo
Copy link
Contributor

Awaiting this merge to fix issue #902 :)
Thanks @an1zhegorodov

@Seldaek
Copy link
Owner

Seldaek commented Dec 13, 2016

From looking at your screenshots @an1zhegorodov it seems to me one important thing that might be worth adding again is the $record['datetime']. It seems it got lost :)

Otherwise I agree it looks much better, thanks for the work.

@mintobit
Copy link
Contributor Author

mintobit commented Dec 13, 2016

@Seldaek Slack has a feature to show date and time below the attachment based on user timezone. That's where datetime is now.

That's what I meant by:

  • Use ts attachment key to display datetime considering user timezone

@Seldaek
Copy link
Owner

Seldaek commented Dec 13, 2016

OK sorry :) Looks fine to me then, thanks!

@Seldaek Seldaek merged commit b732364 into Seldaek:1.x Dec 13, 2016
@danizord
Copy link
Contributor

@Seldaek can you release a tag with this one? :)

@mirfilip
Copy link

mirfilip commented Jan 5, 2017

@an1zhegorodov How is the include_stacktraces switch handled? Let's say I don't use attachments but want to choose whether to include stacktraces.

@mintobit
Copy link
Contributor Author

mintobit commented Jan 6, 2017

@mirfilip Whether it is handled or not depends on the formatter. If you use default formatter (which is LineFormatter), then you can use inlcude_stacktraces. However, the attachment will always include stacktraces, because it uses NormalizerFormatter. Here's a comment I wrote during implementation, might also give you and idea how this works.

@mirfilip
Copy link

mirfilip commented Jan 6, 2017

@an1zhegorodov Stupid me, I was searching for include_stacktraces usage in your PR :( Your comment makes it crystal clear. Again, thx for a good work.

CarsonF added a commit to gmo/common that referenced this pull request Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants