Instrumentation Style Guide

[arielvalentin]

@open-telemetry/ruby-contrib-maintainers This PR feels like it is compensating a bit for a lack of hooks in the request/response execution stack in Net::HTTP like in other libraries like Faraday Middleware.

Unlike with those libraries, these hooks will execute globally for all instances of Net::HTTP and cannot be customized for a specific execution path. I think we need to provide some additional examples for our users on how to extend the functionality of the instrumentations, e.g. writing a custom middleware or using the shared context helpers.

The PR also makes use of private methods, which has a small risk of re-implementing methods that exist in the target library we are patching. This is also true for other instrumentations where we not only define private methods but also constants and instance variables:

• https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb#L52
• https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/trilogy/lib/opentelemetry/instrumentation/trilogy/patches/client.rb#L9

Although this has not been a problem so far, I am concerned we may run into some use cases where a new release of a library will end up conflicting with our patches and may go under the radar for a while.

I would like to get some feedback from y'all about this and see if it makes sense for us to start establishing a more rigid instrumentation style/design guide to help maintainers build more sustainable instrumentations.

Thoughts?

[plantfansam]

This is a great point, and I think you're dead on. I think a style guide for writing instrumentations would be great, as would the examples for custom middleware/shared context helpers. Perhaps the example section of each instrumentation's README should call out what the different examples demonstrate?

[plantfansam]

We talked about this in the sig a tiny bit. Here are some notes.

[ahayworth]

@plantfansam In the interest of not losing track of the suggestions from the google doc, I've copied them in here:

From @robbkidd:

• Performance consciousness
  • Don’t allocate strings willy nilly
  • Bigger methods are OK
• "dont patch private methods in dalli bc the maintainer will crush your dreams”
• Use prepend instead of method aliasing
  • Intermingling the two is recipe for disaster

From @mwear:

• Including instrumentation should not result in new exceptions being raised in any scenarios
  • Explicit or implicit raise
• Flawless, perfect, performant code
• Link to OTEL error handling docs
  • Makes rolling out changes pretty easy

No attribution:

• Hard rules vs. guidelines
• Would be nice to add scaffolding to profile contributions
  • Ruby-bench could be a good thing
  • 1st step could just be providing code snippet for people to use to generate profiling data
• Could be great to have new instrumentations include screenshots of trace views

[ahayworth]

A few thoughts on the notes-from-the-google-doc:

Use prepend instead of method aliasing

Personally I think this is important enough that we should prohibit it via linter (of course, we can allow it on a case-by-case basis). 👍 / 👎 ?

Flawless, perfect, performant code Ruby-bench could be a good thing 1st step could just be providing code snippet for people to use to generate profiling data

We should probably have performance profiling for things anyways... and we should probably have it for the main repo too. Then the instrumentation style guide could be "extend the benchmarking system to cover your new instrumentation and make sure the results are appropriate". Thoughts?

[plantfansam]

@ahayworth i'd be down to lint for prepend; a cursory look doesn't reveal a Rubocop Cop for that (you can prefer either alias or alias_method, but we could write our own (right?) or contribute something upstream.

I made an issue for benchmarking. I don't think adding a benchmarking system should be a blocker to writing the style guide (not to say that you think so, either 😄), but IMO we should add it to style guide if/when we write the benchmarking guide.

[ahayworth]

Agreed on both counts - we could write something for rubocop if we want. And benchmarking shouldn't block the style guide either! :)

[github-actions[bot]]

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.