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: Support slog Logger #155

Closed
wants to merge 14 commits into from
Closed

Conversation

raed-shomali
Copy link
Contributor

@raed-shomali raed-shomali commented Oct 30, 2023

This should take care of #148

@raed-shomali raed-shomali changed the title docs: Improve documentation feat: Support slog Logger Oct 31, 2023
@arusso
Copy link
Collaborator

arusso commented Nov 7, 2023

I think the error in staticcheck is because we're specifying Go 1.18 as the minimum version, but slog isn't available until 1.21. Combined with the new changes in how that file are read in Go 1.21 necessitate updating go.mod to 1.21:

To improve forwards compatibility, Go 1.21 now reads the go line in a go.work or go.mod file as a strict minimum requirement: go 1.21.0 means that the workspace or module cannot be used with Go 1.20 or with Go 1.21rc1.

@raed-shomali
Copy link
Contributor Author

But go.mod has been updated to Go 1.21 as seen in the PR. What am I missing?

Copy link
Collaborator

@arusso arusso left a comment

Choose a reason for hiding this comment

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

I think the usage of logging functions needs correcting, though I question if we're doing the right thing. The whole point of structured logging is to provided logs in a structured format, and I think we're still trying to use it as an unstructured logger which doesn't make much sense.

examples/interaction-middleware/main.go Outdated Show resolved Hide resolved
@arusso
Copy link
Collaborator

arusso commented Nov 7, 2023

But go.mod has been updated to Go 1.21 as seen in the PR. What am I missing?

Oh so it is, let me look at the action a bit more and see why it's not liking that then.

@raed-shomali
Copy link
Contributor Author

As for your comment on the logging, I am not really doing anything special other than pass the arguments to the slog logger.

The default slog logger is setup with a JSON handler but you can change that behavior.

Here are some examples when I was testing:

slacker % go run examples/interaction-middleware/main.go 
{"time":"2023-10-30T22:56:51.22708-04:00","level":"INFO","msg":"connecting to Slack with Socket Mode..."}
2023/10/30 22:56:51 not_authed

And here's some nonsense tests with a Text Handler:

slacker % go run examples/logger/main.go                
time=2023-10-30T22:57:28.498-04:00 level=INFO msg=meow
time=2023-10-30T22:57:28.499-04:00 level=ERROR msg=meow
time=2023-10-30T22:57:28.499-04:00 level=WARN msg=meow
time=2023-10-30T22:57:28.499-04:00 level=INFO msg="connecting to Slack with Socket Mode..."

@arusso
Copy link
Collaborator

arusso commented Nov 7, 2023

Try rebasing on master and re-pushing, hopefully that takes care of the staticcheck errors.

@arusso arusso self-assigned this Nov 7, 2023
@arusso
Copy link
Collaborator

arusso commented Nov 7, 2023

As for your comment on the logging, I am not really doing anything special other than pass the arguments to the slog logger.

My point is that this change doesn't take full advantage of slog, and I think if we're going to support it we should lean into it more. Take for instance the Writer.Delete(), we should embed useful information here so the user does less work ingesting this log data. Something like:

r.logger.Error("failed to delete message", "error", err,  "channel", channel, "message_timestamp", messageTimetamp)
# 2023/11/06 18:22:03 ERROR failed to delete message error="whoops, my bad" channel=CHANNELID message_timestamp=123456...

Of course, this means we're committing to slog since the log module (or anything like it) wont output that properly. If we want to support both, we'll need to do some extra logic to ensure we're properly formatting logs regardless of whether they're expected to be structured or not.

@raed-shomali
Copy link
Contributor Author

Ah! Now I understand your point. Thanks. Will clean up.

arusso and others added 9 commits November 6, 2023 21:59
  Convert BlockID to InteractionID field into the InteractionDefinition struct,
and add the necessary dispatch mechanisms to handle the following interactions:
- shortcut
- message_actions
- view_submission
- view_closed

Add Interaction type to interaction definition
  Convert BlockID to InteractionID field into the InteractionDefinition struct,
and add the necessary dispatch mechanisms to handle the following interactions:
- shortcut
- message_actions
- view_submission
- view_closed

Add Interaction type to interaction definition
  Convert BlockID to InteractionID field into the InteractionDefinition struct,
and add the necessary dispatch mechanisms to handle the following interactions:
- shortcut
- message_actions
- view_submission
- view_closed

Add Interaction type to interaction definition
@raed-shomali
Copy link
Contributor Author

How do you like the changes now?

@arusso
Copy link
Collaborator

arusso commented Nov 23, 2023

Moved to github.com/slack-io/slacker

@arusso arusso closed this Nov 23, 2023
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.

3 participants