Skip to content
This repository was archived by the owner on Jun 11, 2025. It is now read-only.

[KLO-73] Fix/logs websocket#269

Merged
nxtcoder17 merged 3 commits into
release-v1.0.2from
fix/logs-websocket
Feb 14, 2024
Merged

[KLO-73] Fix/logs websocket#269
nxtcoder17 merged 3 commits into
release-v1.0.2from
fix/logs-websocket

Conversation

@nxtcoder17
Copy link
Copy Markdown
Member

@nxtcoder17 nxtcoder17 commented Feb 14, 2024

Resolves kloudlite/kloudlite#32

  • on unsbscribing logs, consumer is deleted, so that next time new consumer comes up, and it streams logs from start

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented Feb 14, 2024

This automated comment suggests enhancements to the PR title and body to improve clarity and facilitate a quicker review

Title suggestion

Ensure clean consumer deletion on WebSocket log unsubscribe
Reasons to update the title
  • Consider specifying the key change or improvement made rather than just stating a general area of fix.
  • The title could be more descriptive about the nature of the fix to provide clarity at a glance.

Body suggestion

This PR addresses an issue where unsubscribing from log streams via WebSocket did not properly delete the consumer, leading to potential resource leaks and incorrect log streaming on new subscriptions. By ensuring the consumer is deleted upon unsubscribe, we prevent the accumulation of unused consumers and guarantee that new subscriptions start streaming logs from the correct starting point. This change involves modifications to the WebSocket server's log handling logic to include a call to `nats.DeleteConsumer` for clean consumer deletion.
Reasons to update the body
  • Expand on the description to include why this change was necessary. What issue does it solve?
  • Mention any potential impacts or side effects this change might have.
  • It would be helpful to briefly describe how the fix was implemented.
  • Consider adding any relevant links or references to related discussions or issues for context.

Benefits of a great title and description

Author benefits

  • Faster Approval Times: Clear descriptions lead to quicker, more efficient code review processes.
  • Higher Quality Reviews: Well-crafted descriptions lead to more insightful feedback, improving the overall quality of the code.
  • Easier Future Maintenance: Simplifies debugging and updating code by providing context and rationale.

Reviewer benefits

  • Efficient Review Process: Concise, informative descriptions enable quicker understanding and assessment of changes.
  • Improved Decision-Making: Detailed context aids in evaluating the impact and necessity of the change.
  • Facilitates Knowledge Sharing: Offers insights into codebase evolution, design choices, and problem-solving approaches.

Guide: Writing good PR descriptions - Google

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: This pull request introduces changes aimed at improving the handling of log subscriptions in a WebSocket server. Specifically, it ensures that upon unsubscribing, the associated consumer is deleted. This action allows for a fresh consumer to be created for new subscriptions, which will stream logs from the start. The changes span across multiple files, including adjustments to the domain logic for handling WebSockets and logs, as well as modifications to the Jetstream consumer setup within the NATS messaging package. Additionally, there's a minor update to the Taskfile.yml to reflect a change in the image prefix used in the build process.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
  • Unsupported files: the diff contains files that Sourcery does not currently support during reviews.

General suggestions:

  • Ensure comprehensive testing around the new logic for deleting consumers to prevent potential issues with consumer management and log streaming.
  • Consider the broader impact of these changes on the system, especially in terms of resource management and error handling. It might be beneficial to implement additional safeguards or checks to ensure that consumers are only deleted when it's safe to do so.
  • Review the changes to logging messages and ensure they are consistent and informative, providing enough context for debugging without being overly verbose.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@nxtcoder17 nxtcoder17 merged commit 5dd1b70 into release-v1.0.2 Feb 14, 2024
@nxtcoder17 nxtcoder17 deleted the fix/logs-websocket branch February 14, 2024 07:30
nxtcoder17 added a commit that referenced this pull request Feb 14, 2024
@nxtcoder17 nxtcoder17 changed the title Fix/logs websocket [KlO-73] Fix/logs websocket Feb 14, 2024
@nxtcoder17 nxtcoder17 changed the title [KlO-73] Fix/logs websocket [KLO-73] Fix/logs websocket Feb 14, 2024
abdheshnayak pushed a commit that referenced this pull request Nov 5, 2024
abdheshnayak pushed a commit that referenced this pull request Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[KLO-73] App logs not working in production

1 participant