-
Notifications
You must be signed in to change notification settings - Fork 29
feat(insights): automatic insights instrumentation #215
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
Conversation
* Instrumenting ASGI * Add celery events & reworked tests * Django request & db events * Flask request & db instrumentation * ASGI instrumentation * Syntax update * Updated README * Change "component" to "app" for insights event I followed the naming from our notice data. The more common name for this field is "app", so renaming to that. * Remove url & add blueprint to flask events * Removing args from celery event Until we have filtering and before_event functionality. * Add duration to celery events * PR fixes
Also fix config bleeding issue with asgi test
* feat(insights): add config for insights contrib This also reworks our config class to use a root dataclass with nested dataclasses for each `insights_config` option set. * Refactor the missing key error handling * Add ASGI config * README fixes * Add comments back and fixed readme, remove cruft * Log errors Also a nice little refactor on the Configuration internals. * Wrap insights event code in a try * How about some test results * Fix 3.8 object is not subscriptable error * Don't reference private Context * Check code-quality as well
* refactor threadlocals This now uses the ContextVar and a helper ContextStore class. * Basic event_context * More tests * Fix other Notice usage
…t context to celery tasks (#216) * feat(insights) add request_id * Add request_id to notice * Fix test * Fix formatted string * Update honeybadger/contrib/celery.py Co-authored-by: Copilot <[email protected]> * Add logger to celelry * Sanitize request_id from inputs --------- Co-authored-by: Copilot <[email protected]>
| self.config = Configuration() | ||
| self.thread_local = threading.local() | ||
| self.thread_local.context = {} | ||
| self.events_worker = EventsWorker( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to create the events_worker if insights is not enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to build the worker on boot and then enable and disable it via config change (which also gets passed into the worker), as some apps are configured after the init call.
|
Please note: Also, setting This isn’t necessarily a blocker, but it might be worth considering more flexible configuration options down the line. |
|
I really appreciate you taking a close look at this one @subzero10!
Yeah, we don't have an explicit config for disabling all events submission. I think it would be worth giving users that control, but we probably should discuss that in our client spec, as we should have parity with all the clients if we want to allow that kind of option. |
This is currently a WIP PR to keep track of the work being merged into this preview branch.