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 an Opbeat Tracer #262

Merged
merged 1 commit into from
Oct 30, 2016

Conversation

olleolleolle
Copy link
Contributor

@olleolleolle olleolleolle commented Oct 27, 2016

This PR adds a new autoloaded Tracer implementation for Opbeat.

  • load it only if Opbeat is defined (this is taken care of just like newrelic_rpm is required in the file that is autoloaded)
  • ask @mikker if this is the way to use Opbeat.trace

@mikker
Copy link

mikker commented Oct 28, 2016

Nice! I don't know Hutch – but I noticed one thing. You need to wrap the whole thing in a transaction. Translations have traces. In Rails for example, every action is wrapped in a Transaction, and the DB calls are traces.

Opbeat.transaction(sig, KIND, extra: extra_from(message)) do
  @klass.process(message)
end.done(true)

Be aware that the Opbeat client might need to be started manually if running in another process than Rails (where it's started during boot).

@olleolleolle
Copy link
Contributor Author

@mikker Is this how to use trace/transaction?

sig, KIND, extra: extra_from(message)
) { @klass.process(message) }
::Opbeat.trace(sig, KIND, extra: extra_from(message)) {
::Opbeat.transaction(sig, KIND, extra: extra_from(message)) do
Copy link

Choose a reason for hiding this comment

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

Other way around 😄 Transaction(Trace())

@olleolleolle
Copy link
Contributor Author

@mikker Thanks for checking my work. Re-check?


def extra_from(message)
{
body: message.body.to_s,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider: Make this scrubbable, somehow?

Copy link

Choose a reason for hiding this comment

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

Scrubbable how? You don't need the trace, actually. Transaction automatically have one root trace, so adding another one is probably redundant. I can see how I could've let you to add one. Sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the note on the trace, I'll remove that inner bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On scrubbing: As we may send confidential information in a Message, its body is perhaps not for everyone's eyes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scrubbing feature is way outside the scope of this PR. (Use-cases include: Anonymize hospital person data, for instance. Name=Jane Doe, that kind of thing.)

Copy link
Member

Choose a reason for hiding this comment

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

Data scrubbing is arguably outside of Hutch's scope. If you are serious about this, use TLS and payload encryption, then scrub as much as you need in your own app.

@olleolleolle olleolleolle changed the title Attempt at Opbeat tracer Add an Opbeat Tracer Oct 28, 2016
@olleolleolle
Copy link
Contributor Author

@michaelklishin What's your preferred way to "only ever load this if defined?(Opbeat)"?

@olleolleolle
Copy link
Contributor Author

The JRuby seems to have had a hiccup on an unrelated test.

@michaelklishin
Copy link
Member

@olleolleolle Kernel#autoload? just a condition with a require? I don't have a strong opinion.

@michaelklishin michaelklishin self-assigned this Oct 30, 2016
@michaelklishin michaelklishin merged commit ee1e054 into ruby-amqp:master Oct 30, 2016
@michaelklishin
Copy link
Member

Thank you!

@mikker
Copy link

mikker commented Oct 30, 2016

Thank you for this! 👍 (cc @roncohen)

@olleolleolle olleolleolle deleted the feature/opbeat-tracer branch October 30, 2016 17:17
@olleolleolle
Copy link
Contributor Author

Go team! So many incremental baby-steps, so many helpful nudges and high-fives.

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