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

Confusing debug logs by ConversationStore #629

Closed
Mogadampalli-Jayanth opened this issue Sep 16, 2020 · 4 comments · Fixed by #827
Closed

Confusing debug logs by ConversationStore #629

Mogadampalli-Jayanth opened this issue Sep 16, 2020 · 4 comments · Fixed by #827
Assignees
Labels
discussion M-T: An issue where more input is needed to reach a decision

Comments

@Mogadampalli-Jayanth
Copy link

Mogadampalli-Jayanth commented Sep 16, 2020

I am developing one bot
It is throwing error when I send message to bot
This is the following error:
bolt-app Conversation context failed loading for ID:<channeID>, error: Conversation not found
Please provide solution to resolve this error!

@gitwave gitwave bot added the untriaged label Sep 16, 2020
@seratch
Copy link
Member

seratch commented Sep 16, 2020

@Mogadampalli-Jayanth Thanks for flagging this! I do understand this debug message is confusing particularly for developers that don't use the conversation store. https://github.com/slackapi/bolt-js/blob/%40slack/bolt%402.3.0/src/conversation-store.ts#L67

@stevengill @aoberoi (and other maintainers)
Considering the situation where most developers do not use the feature, I think it's about time to reconsider the debug logs caused by conv store. Thoughts?

@seratch seratch added discussion M-T: An issue where more input is needed to reach a decision and removed untriaged labels Sep 16, 2020
@seratch seratch changed the title throwing error when sending message with bot Confusing debug logs by ConversationStore Sep 16, 2020
@vvvprabhakar
Copy link

+1

@aoberoi
Copy link
Contributor

aoberoi commented Sep 20, 2020

Thanks for starting this discussion, I think it’s too confusing too!

Do you think we should no longer add the conversationContext middleware by default? This change would require a major version change, and I don’t think this is high enough impact for the project to trigger the next major version to happen. Therefore this may take several months. It would also decrease compatibility for users migrating from Hubot, and we’d like to keep the compatibility high since there will be a wave of developers migrating as new Platform features are released in the next 3 - 6 months.

Do you think we should remove this log line completely? For a developer who is using the conversationContext middleware, we need to offer some means to debug and understand the situation where the ConversationStore doesn’t return any data. Do we have another idea for how we should do this?

Should we explore “scoped” loggers? In theory, using the DEBUG level means most people shouldn’t see this log line unless they are actively debugging. But in practice, people who are actively debugging are mostly not even aware that the conversationContext middleware exists, which leads to this confusion. We also have a general problem where the inheritance of loggers from App to other objects (e.g. ExpressReceiver, InstallProvider, even WebClient) is not intuitive or clear. Maybe this problem is closely related in that log levels are not enough to describe which parts of the application you expect to hear from. Do we have any ideas?

@seratch
Copy link
Member

seratch commented Nov 5, 2020

related issue: #680

seratch added a commit to seratch/bolt-js that referenced this issue Mar 10, 2021
@seratch seratch self-assigned this Mar 10, 2021
misscoded added a commit that referenced this issue Mar 10, 2021
…-logs

Fix #629 confusing debug log by ConversationStore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion M-T: An issue where more input is needed to reach a decision
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants