Skip to content

feat(insights): event_context #214

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 5 commits into from
May 28, 2025
Merged

Conversation

rabidpraxis
Copy link
Contributor

This adds a new set of functions to the Honeybadger class for setting and applying event context.

I also refactored the internals of storing context quite a bit:

  • Replaced the threadlocals with a ContextStore class that uses ContextVar, which is the recommended way to manage thread local variables in a 3.7+ world.
  • Used ContextStore to create both notice and event context stores
  • Updated the core methods to delegate to the new instances
  • I refactored the Notice class by removing the thread local code, opting to just send in the context & tag variable, keeping it just a pure rendering class.

This now uses the ContextVar and a helper ContextStore class.
@rabidpraxis rabidpraxis requested review from roelbondoc, subzero10, a team and Copilot May 23, 2025 20:28
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a new event context API and refactors how context is stored using ContextVar-backed stores.

  • Introduce ContextStore for thread- and async-safe context management
  • Add event context methods (set_event_context, reset_event_context, event_context) and merge them in Honeybadger.event
  • Refactor Notice to remove thread locals and improve tag parsing

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pytest.ini Configure default fixture loop scope for asyncio tests
dev-requirements.txt Add pytest-asyncio
honeybadger/context_store.py New ContextStore using ContextVar
honeybadger/core.py Replace threadlocals with ContextStore; add event-context APIs
honeybadger/notice.py Remove thread-local context processing; enhance tag parsing
honeybadger/protocols.py Rename protocol parameter from payload to notice
honeybadger/connection.py Remove unused Protocol import
honeybadger/types.py Clean up unused imports
honeybadger/tests/test_core.py Add tests for async/thread isolation and event-context behavior
honeybadger/tests/test_notice.py Update notice-tag tests
honeybadger/tests/contrib/test_asgi.py Adjust ASGI plugin tests for new event-context handling
Comments suppressed due to low confidence (3)

honeybadger/tests/contrib/test_asgi.py:6

  • [nitpick] Importing honeybadger this way is ambiguous; consider importing the module or class explicitly (e.g. import honeybadger as hb or from honeybadger import Honeybadger) to clarify what’s being referenced.
from honeybadger import honeybadger

honeybadger/notice.py:55

  • The _get_thread_context method is no longer used after removing thread-local support; consider removing this dead code to keep the class focused.
    def _get_thread_context(self):

honeybadger/protocols.py:6

  • [nitpick] The parameter name was changed from payload to notice; update any docstrings or inline comments to reflect this rename for clarity.
    def send_notice(self, config: Any, notice: Notice) -> Optional[str]:

Copy link
Contributor

@roelbondoc roelbondoc left a comment

Choose a reason for hiding this comment

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

This looks good, I do like how this simplifies the Notice class. There is one more Notice class initialization that needs to be updated here.

@rabidpraxis
Copy link
Contributor Author

There is one more Notice class initialization that needs to be updated

Great catch @roelbondoc !

961eea7

@rabidpraxis rabidpraxis merged commit ee20e94 into insights-instrumentation May 28, 2025
34 checks passed
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