-
Notifications
You must be signed in to change notification settings - Fork 251
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
feat: add instrumentation for rspec #981
feat: add instrumentation for rspec #981
Conversation
24b182b
to
bb104ea
Compare
Is there any interest in review/accepting this instrumentation? I can make it a standalone project if not. |
@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:
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/ |
bb104ea
to
8c1cecf
Compare
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 :) |
Thanks both
@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.
I generally agree about inclusion in
I will factor this into the README when I get to writing it. Is there anything besides commentary around |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
instrumentation/rspec/README.md
Outdated
@@ -0,0 +1,52 @@ | |||
# OpenTelemetry Rspec Instrumentation | |||
|
|||
Todo: Add a description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo :)
instrumentation/rspec/lib/opentelemetry/instrumentation/rspec/version.rb
Outdated
Show resolved
Hide resolved
instrumentation/rspec/lib/opentelemetry/instrumentation/rspec/formatter.rb
Outdated
Show resolved
Hide resolved
instrumentation/rspec/lib/opentelemetry/instrumentation/rspec/formatter.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
instrumentation/rspec/lib/opentelemetry/instrumentation/rspec/formatter.rb
Outdated
Show resolved
Hide resolved
instrumentation/rspec/lib/opentelemetry/instrumentation/rspec/formatter.rb
Outdated
Show resolved
Hide resolved
instrumentation/rspec/lib/opentelemetry/instrumentation/rspec/formatter.rb
Outdated
Show resolved
Hide resolved
instrumentation/rspec/lib/opentelemetry/instrumentation/rspec/formatter.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This is done to protect against tools that override `Time::now` and similar methods.
064db83
to
fada1ae
Compare
@ericmustin, I think I'm finished with making changes. Please merge when convenient. |
@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! |
|
||
## TODO: Include the supported version to be tested here. | ||
## Example: | ||
# appraise 'rspec-3.10' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you planning to uncomment this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 🥇 !! Sending my fake approval
instrumentation/rspec/lib/opentelemetry/instrumentation/rspec/formatter.rb
Show resolved
Hide resolved
instrumentation/rspec/opentelemetry-instrumentation-rspec.gemspec
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
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 :
::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.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