feat: add instrumentation for rspec

[chrisholmes]

This PR covers adding instrumentation for RSpec using a RSpec formatter. It could then be used as part of a CI/CD pipeline that is configured to emit traces. At this point, parent context would need to passed manually from other processes, but I understand this being discussed as an addition to the spec.

There are two ways to enable instrumentation either by configured Otel to use the instrumentation or by adding the formatter to RSpec. You can see examples of this in examples/.

Some things :

• The formatter is using ::attach and ::detach and a stack to add and remove span context. I think this is appropriate considering how example and example groups are currently executed, but I'm assuming this might break if a later RSpec version offers multi-threaded execution.
• Should test framework instrumentation be auto-instrumented?
• I've had to add a monkey patch at the test/rspec_patches.rb so that RSpec doesn't clobber minitest's #describe method. This might not be needed if specs were written in RSpec, but I don't think this is a good idea as there would then be multiple test frameworks used in this repo.

Also, the README is still a WIP

[chrisholmes]

Is there any interest in review/accepting this instrumentation? I can make it a standalone project if not.

[ericmustin]

@chrisholmes hmm, I think so? I'd like to defer to @robertlaurin @fbogsany and @mwear here.

There's a number of open PRs that need some love, so I don't have a firm timeline, but can make a best efforts attempt to give a review next week.

Off the top of my head, some common footguns with instrumentation for ci/cd+test frameworks are:

• timecop is often used within ruby tests and can "mess" with the accuracy of span duration, we'll want to offer a mechanism to account for that.
• we probably don't want to include this in the -all instrumentation gem, or if we do, have some separate flag for .use_all for ci/cd instrumentation. Just my two cents, maybe there ought to be specification about this in an OTEP.
• From what I've seen from my time previously at a vendor who had a ci/cd instrumentation offering, there needs to be some clear documentation around how to correctly setup processors and exporters to ensure that all spans are "flushed" from a trace for a test.

It's possible some of your work already accounts for this stuff (i haven't reviewed it yet), but just thinking out loud about some areas of coverage to provide robust ci/cd or test framework instrumentation.

If you do choose to host as a separate project for now(which is totally fine, as you may have something that works for you faster than something that can work as a general solution), one thing I would say is that to add that project to the OpenTelemetry Registry: https://opentelemetry.io/registry/

[robertlaurin]

Is there any interest in review/accepting this instrumentation? I can make it a standalone project if not.

Sorry for the slow reviews on my part, I will carve out time today. The silence was not intended to convey disinterest.

This is a really interesting PR :)

[chrisholmes]

Thanks both

• timecop is often used within ruby tests and can "mess" with the accuracy of span duration, we'll want to offer a mechanism to account for that.

@ericmustin I hadn't considered this. RSpec has an internal clock for this reason, but I have added a clock for this instrumentation as I find it makes the code simpler.

• we probably don't want to include this in the -all instrumentation gem, or if we do, have some separate flag for .use_all for ci/cd instrumentation. Just my two cents, maybe there ought to be specification about this in an OTEP.

I generally agree about inclusion in -all, but I'm keen to understand what others think.

• From what I've seen from my time previously at a vendor who had a ci/cd instrumentation offering, there needs to be some clear documentation around how to correctly setup processors and exporters to ensure that all spans are "flushed" from a trace for a test.

I will factor this into the README when I get to writing it. Is there anything besides commentary around at_exit handlers and calling shutdown that should be documented?

[robertlaurin]

This PR made for a fun discussion on the SIG, the main thing I'd like to see added is support for supplying a tracer provider to the formatter.

[robertlaurin]

Todo :)

[ericmustin]

Sorry for the delay in reviewing here, apologies.

I think, this is super close imo. Great work, appreciate the awesome contribution.

Since we're treating this as experimental i'm in favor of avoiding as much bikeshedding as possible over stuff like span attribute naming conventions or whatever, and just nailing down the concrete bits and getting this shipped. Most of my feedback falls into the category of nits and i'm open to what you think are the right choices here.

Setting span status if the result.status is a failing test looks like the only missing bit here to me. Once that's done and the other comments have either been addressed or resolved according to your best judgement, I'd like to approve this and get it out the door.

[ericmustin]

Lgtm! ty for implementing that feedback. As discussed, ideally we'd set the status description as well on error, but not blocking imo. Looks like a small conflict with main but rebasing ought to clean that up, otherwise keen to see this merged and released.

[chrisholmes]

@ericmustin, I think I'm finished with making changes. Please merge when convenient.

[ericmustin]

@chrisholmes i'm not a maintainer so i can't merge, i've asked for another review here and then we'll get this merged. Great work again, thanks!

[SomalianIvan]

are you planning to uncomment this?

[SomalianIvan]

This is 🥇 !! Sending my fake approval

[robertlaurin]

🥇