Skip to content

Add ActionMailer support#34

Open
xeviknal wants to merge 1 commit intobmista:masterfrom
xeviknal:action-mailer
Open

Add ActionMailer support#34
xeviknal wants to merge 1 commit intobmista:masterfrom
xeviknal:action-mailer

Conversation

@xeviknal
Copy link
Copy Markdown

@xeviknal xeviknal commented Feb 1, 2018

Adding support for ActionMailer. It adds an span with the mailer's name.

@bmista bmista self-assigned this Feb 5, 2018
@bmista bmista self-requested a review February 5, 2018 19:16
@bmista bmista added this to the v0.6.0 milestone Feb 5, 2018
tracer: tracer,
active_span: active_span,
start_time: start,
message_id: payload.fetch(:message_id),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

what about other information like data, bcc etc which is supplied within the event? I assume that was the only information necessary for your use case?

'component' => 'ActionMailer',
'span.kind' => 'client',
'mail.message_id' => fields.fetch(:message_id),
'mail.to' => fields.fetch(:to),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

to, and from are arrays. From OpenTracing specification

The tag value, which must be either a string, a boolean value, or a numeric type

Probably what needs to happen is to format it into a single string value

it "sets standard OT tags" do
[
['component', 'ActionMailer'],
['span.kind', 'client']
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm missing a test for non-standard OT tags

@bmista
Copy link
Copy Markdown
Owner

bmista commented Feb 5, 2018

btw @xeviknal hello, and thanks for your first contribution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants