feat: implements honeybadger.event by synchronous log call #512
Conversation
halfbyte
changed the title
There was a problem hiding this comment.
Good start! Check out my comment about the event method signature (the spec may need to be updated to match). @rabidpraxis can provide additional input if needed.
Once we have the method signature and event schema worked out, the next step should be to build an abstraction for the event transport—the first of which can be a debug transport that makes the logger.debug call you're making now. You should take a look at our current backend implementation, which already does something similar. I'm not sure if we can/should use that in any way. It could be a nice approach though, so people don't have to configure two different types of backend.
@stympy @subzero10 @rabidpraxis thoughts on that?
I think it would be nice to have one backend configuration instead of two. |
|
It's more of a rehashing of what we have here, but I updated the spec with example JSON payload structures for both "event" and "log" events. |
|
@joshuap I've changed the arguments and the log structure and added a timestamp (As I assume this needs to be done as closest to the method call as possible to be accurate) I'll go on and add a backend debug implementation and some non-functional implementations in the other backends in a separate PR |
* Implement simple debug backend endpoint for events This currently is missing a queue and calls the backend directly from the agent. Should I implement an events_worker within this PR or in the PR that adds the server backend? * Refactor signature of events backend to take only one argument * WIP: Add worker * WIP start of worker spec * Worker spec successfully duplicated * Implement timeout mechanism using separate thread Given that the worker relies on the Queue as the main scheduling mechanism I saw no other way than to start a second thread that occasionally throws a message into the queue to check if the timeout is reached. This seems to work in testing. * Remove one timeout check, namespace config * Remove unused code * Add events worker to agent stop/flush commands * Fix debug message in events worker --------- Co-authored-by: Joshua Wood <josh@joshuawood.net>
There seems to be a slight difference in how sleep works in jruby so the timeouts in the tests did not hit predictably.
|
Ah, I didn't realize that the testsuite would not run on my sub-PR (@subzero10 is this intentional? It seems that only PRs against There are a couple of failures, some of them related to jruby handling threads and sleep a bit different (No real surprises here). I found at least one issue that seems an unrelated problem, but I'll have a look and will try to fix these before creating the PR for the http transport. |
Yes, it's intentional but you can change it if you want to and have it run for all PRs on any target branch. |
* Implement simple debug backend endpoint for events This currently is missing a queue and calls the backend directly from the agent. Should I implement an events_worker within this PR or in the PR that adds the server backend? * Refactor signature of events backend to take only one argument * WIP: Add worker * WIP start of worker spec * Worker spec successfully duplicated * Implement timeout mechanism using separate thread Given that the worker relies on the Queue as the main scheduling mechanism I saw no other way than to start a second thread that occasionally throws a message into the queue to check if the timeout is reached. This seems to work in testing. * Remove one timeout check, namespace config * Remove unused code * Add server back end functionality for events This adds a minimal set of tests to ensure API conformance I've tested the code manually against "the real thing(tm)" * Add events worker to agent stop/flush commands * Fix debug message in events worker --------- Co-authored-by: Joshua Wood <josh@joshuawood.net>
There was a problem hiding this comment.
I tested this out in a new Rails 7.1 app, and was reporting Ahoy events to Honeybadger in about 10 minutes from rails new. Magical. :)
Since this doesn't integrate with anything by default, I'm fine with shipping it in a minor gem release whenever we're ready. I want more reviews first, and @stympy or myself will do the final merge.
This enables both signatures:
# With event type as first argument (recommended):
Honeybadger.event("user_signed_up", user_id: 123)
# With just a payload:
Honeybadger.event(event_type: "user_signed_up", user_id: 123)
The config is initialized after the agent is created (when the app loads).
This results in less change for current users—if you aren't using insights, the extra threads don't need to run. We could change this back in the future.
This is "the simplest thing that could possibly work" without the "could possibly work" part.
I want make sure 2 things with this PR in this state:
Honeybagder::eventsignature sufficientNext step would be (can be in this PR or separate) to implement
Honeybadger::loggerbut sinceHoneybadger::loggercurrently already points to the internal logger system, this would need some refactoring.