-
Notifications
You must be signed in to change notification settings - Fork 226
feat: Add FactoryBot instrumentation #1721
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 FactoryBot instrumentation #1721
Conversation
|
Hi @americodls, thanks for opening this PR. Would you be interested in being a codeowner for this gem? That would involve reviewing PRs and issues related to the factory bot instrumentation. We don't have the codeowner behavior automated in GitHub, but we're currently documenting this in the CODEOWNERS file in the repo. |
Adds OpenTelemetry instrumentation for the FactoryBot gem to provide
visibility into test data creation patterns during testing.
Subscribes to the factory_bot.run_factory ActiveSupport::Notifications event
to capture all factory operations without monkey-patching. Supports FactoryBot
4.0+ and captures all factory strategies (create, build, build_stubbed,
attributes_for) including batch operations (create_list, build_list, etc.).
Span naming: FactoryBot.{strategy}({factory_name})
Example: FactoryBot.build(user), FactoryBot.create_list(post)
Span attributes:
- factory_bot.strategy: Internal strategy name (build, create, stub, attributes_for)
- factory_bot.factory_name: Name of the factory being used
- factory_bot.traits: Comma-separated list of traits applied (if any)
b21de9a to
48f77b0
Compare
|
Hi @kaylareopelle! Happy to take it on 🙂 |
|
@americodls would you mind to share more how do you instrument testing also? |
| span_name = "FactoryBot.#{strategy_symbol}(#{factory_name})" | ||
|
|
||
| attrs = { | ||
| 'factory_bot.strategy' => internal_strategy, |
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.
Suggestion name this test.data.strategy given it could be used elsewhere and we already have the test namespace?
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.
Thanks for the feedback!
I checked the OpenTelemetry test semantic conventions and found that the test.* namespace currently only defines attributes for test execution metadata (test cases, suites, and their statuses).
Since test.data isn’t part of the spec yet, using it now could risk conflicts if it’s added later. The current factory_bot.* approach keeps things scoped to this library’s specific domain.
I’d suggest keeping the dedicated namespace for now — we can always generalize to something like test.data.* if similar fixture/factory patterns start appearing across other testing libraries. That way, we’re being intentional rather than speculative about the abstraction.
Of course, I’m open to other perspectives — input from the maintainers would be especially valuable given their broader experience in this space.
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.
So perhaps the following helps defining attributes
When defining an attribute for a narrow use case, think about potential broader use cases. For example, if creating a system-specific attribute, evaluate whether other systems in the same domain might need a similar attribute in the future.
I see the attributes identified as falling into that category as anything generating data needs a strategy etc.
I would even suggest raising a pr to get the attributes/spans added.
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.
Although I agree with @thompson-tomo that we need to do our best to adhere to the spec for attribute naming; I am amenable to releasing custom attributes for our instrumentations since they are all pre-1.0 at this point and we are still working through figuring out how the version semantic conventions used in instrumentation libraries.
The factory bot use case seems very narrow since the spans are implementation specific. Other testing object mother or factory libraries will likely use different domain language based on DSLs and helper methods. We may find that higher level span names may have some overlap but library specific attributes like traits are likely not going to be a good fit for a "broader" use case.
@americodls Are there any other testing libraries that are currently instrumented in other languages?
Is there enough traction there to start up a conversation between SIGs?
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.
@americodls Are there any other testing libraries that are currently instrumented in other languages?
Is there enough traction there to start up a conversation between SIGs?
I haven't found anything that's why I suggested to "go as is".
| 'factory_bot.factory_name' => factory_name.to_s | ||
| } | ||
|
|
||
| attrs['factory_bot.traits'] = traits.join(',') if traits.any? |
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.
Suggestion name this test.data.traits given it could be used elsewhere and we already have the test namespace?
...entation/factory_bot/lib/opentelemetry/instrumentation/factory_bot/run_factory_subscriber.rb
Outdated
Show resolved
Hide resolved
| attrs = { | ||
| 'factory_bot.strategy' => internal_strategy, | ||
| 'factory_bot.factory_name' => factory_name.to_s | ||
| } | ||
|
|
||
| attrs['factory_bot.traits'] = traits.join(',') if traits.any? |
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.
If test.data.* is adopted, I would add a test.data.provider.name attribute.
Something like this: # spec/spec_helper.rb
#...
if ENV["ENABLE_TRACING"]
require 'opentelemetry/sdk'
require 'opentelemetry/instrumentation/rspec'
OpenTelemetry::SDK.configure do |c|
# ...
c.use 'OpenTelemetry::Instrumentation::RSpec'
c.use 'OpenTelemetry::Instrumentation::FactoryBot'
# ...
end
end
RSpec.configure do |config|
#...
config.after(:suite) do
OpenTelemetry.tracer_provider.shutdown if ENV["ENABLE_TRACING"]
end
#...
end |
arielvalentin
left a comment
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.
Thank you for your submission!
There are a few required changes you will have to make to avoid issues/conflicts with our release tooling.
I will have to reach out for help to other users who are FactoryBot users to help with a review before we merge.
instrumentation/factory_bot/lib/opentelemetry/instrumentation/factory_bot/version.rb
Outdated
Show resolved
Hide resolved
instrumentation/factory_bot/opentelemetry-instrumentation-factory_bot.gemspec
Outdated
Show resolved
Hide resolved
da05c9e to
4d0543b
Compare
3e4df45 to
30a3ea8
Compare
…factory_bot/version.rb Co-authored-by: Ariel Valentin <[email protected]>
…ory_bot.gemspec Co-authored-by: Ariel Valentin <[email protected]>
Co-authored-by: Ariel Valentin <[email protected]>
Per OpenTelemetry semantic conventions, plural attribute names should use array values. Changed factory_bot.traits from comma-separated string to string array.
- Move instrumentation.install to top-level before block - Remove redundant install calls from nested describe blocks - Add comprehensive attribute tests for all strategies - Test empty traits array in all relevant sections - Ensure all 3 attributes tested: strategy, factory_name, traits
30a3ea8 to
9e42630
Compare
|
@americodls there seems to be conflicts with upstream. Please merge main and we will proceed. |
arielvalentin
left a comment
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.
@americodls thank you again for your submission.
Although I approved this PR, I am going to have request some additional changes.
Our CI workflows have changed and this gems test suite is not longer running by default.
You will need to add entries in the following actions workflow:
paths: &path_filters
In addition to that please add factorybot as a gem dependency in our canary install test:
| gem 'faraday' |
Thank you for your patience.
|
Thanks for your feedback! Let me know if there’s anything else you’d like me to tweak! |
Adds OpenTelemetry instrumentation for the FactoryBot gem to provide visibility into test data creation patterns during testing.
Subscribes to the factory_bot.run_factory ActiveSupport::Notifications event to capture all factory operations without monkey-patching. Supports FactoryBot 4.0+ and captures all factory strategies (create, build, build_stubbed, attributes_for) including batch operations (create_list, build_list, etc.).
Span naming: FactoryBot.{strategy}({factory_name})
Example: FactoryBot.build(user), FactoryBot.create_list(post)
Span attributes: