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

Add honeybadger_events appender #280

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

trvsdnn
Copy link
Contributor

@trvsdnn trvsdnn commented Jun 11, 2024

Changelog

Entry added:

  • Add appender for Honeybadger events/insights API

Description of changes

Adds an additional Honeybadger appender honeybadger_events for their events/insights API.

The appender uses the honeybadger gem and the Honeybadger.event method, which is intelligent about batching log lines and sending a single request.

@stympy
Copy link
Contributor

stympy commented Jun 12, 2024

I wonder if it would make sense to modify the existing appender instead of creating a new one. The non-exception branch of the log method could use Honeybadger.event to send the payload.

@trvsdnn
Copy link
Contributor Author

trvsdnn commented Jun 12, 2024

I considered that – it's a bit odd to use semantic_logger for exceptions when the honeybadger gem does it already. Unless I'm missing some clear benefit.

The problem IMO is that it's a change in behavior for anyone using it and would/should require a major release I would think.

This approach felt similar to the NewRelic vs NewRelicLogs appenders.

@stympy
Copy link
Contributor

stympy commented Jun 12, 2024

I considered that – it's a bit odd to use semantic_logger for exceptions when the honeybadger gem does it already. Unless I'm missing some clear benefit.

Agreed :)

The problem IMO is that it's a change in behavior for anyone using it and would/should require a major release I would think.

This approach felt similar to the NewRelic vs NewRelicLogs appenders.

Yeah, that makes sense.


# Use Raw Formatter by default
def default_formatter
SemanticLogger::Formatters::Raw.new(time_key: :ts, time_format: :rfc_3339)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SemanticLogger::Formatters::Raw.new(time_key: :ts, time_format: :rfc_3339)
SemanticLogger::Formatters::Json.new(time_key: :ts, time_format: :rfc_3339)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honeybadger.event seems to use a string argument as the event_type and not treat it as an object.

Honeybadger.event('{"foo":"bar"}')

results in

@ts "2024-06-12 19:34:05.000"
event_type "{"foo":"bar"}"

vs

Honeybadger.event({foo:"bar"})
@ts "2024-06-12 19:37:29.000"
foo "bar"

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right, ignore me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't even know it accepted a string... good to know!

Copy link
Owner

Choose a reason for hiding this comment

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

Looking at the Honeybadger documentation for the event api, the first argument is the event_type, and the second argument is a Hash. https://docs.honeybadger.io/lib/ruby/getting-started/sending-events-to-insights/#manually-sending-events

We could make the event_type the value from log.metric when present and something like event.semantic_logger when a log entry has no metric. Following the examples from the documentation: https://docs.honeybadger.io/lib/ruby/getting-started/sending-events-to-insights/#event-captures

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s totally fine to pass a hash as the first argument and not specify an event type, if that makes more sense. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels right to send event_type for instrumentation type events like process_action.action_controller – using the honeybadger gem to handle instrumentation gives you that.

For raw logging I don't like the idea of the appender deciding what you want to include in the log payload.

@reidmorrison
Copy link
Owner

Understandably we are calling it Honeybadger Events, since it uses the Event API, but they refer to overall solution as Honeybadger Insights: https://docs.honeybadger.io/guides/insights/
Can we rename the appender to HoneybadgerInsights ?

For anyone that first comes across the appender, Honeybadger Insights would be simpler to understand.

@stympy
Copy link
Contributor

stympy commented Jun 29, 2024

Understandably we are calling it Honeybadger Events, since it uses the Event API, but they refer to overall solution as Honeybadger Insights: https://docs.honeybadger.io/guides/insights/ Can we rename the appender to HoneybadgerInsights ?

For anyone that first comes across the appender, Honeybadger Insights would be simpler to understand.

👍

@reidmorrison reidmorrison merged commit 003b97c into reidmorrison:master Jul 4, 2024
5 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