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.

Sign up for GitHub

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

Instrument bundler version #3283

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Instrument bundler version #3283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions bundler/lib/dependabot/bundler/file_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ def parse
dependency_set += gemspec_dependencies
dependency_set += lockfile_dependencies
check_external_code(dependency_set.dependencies)
instrument_package_manager_version
dependency_set.dependencies
end

Expand All @@ -42,6 +43,17 @@ def git_source?(dependencies)
end
end

def instrument_package_manager_version
version = Helpers.detected_bundler_version(lockfile)
Dependabot.instrument(
Notifications::FILE_PARSER_PACKAGE_MANAGER_VERSION_PARSED,
ecosystem: "bundler",
package_managers: {
"bundler" => version
}
)
end

def gemfile_dependencies
dependencies = DependencySet.new

Expand Down
7 changes: 7 additions & 0 deletions bundler/lib/dependabot/bundler/helpers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ module Helpers
def self.bundler_version(_lockfile)
V1
end

def self.detected_bundler_version(lockfile)
return "unknown" unless lockfile
return V2 if lockfile.content.match?(/BUNDLED WITH\s+2/m)

V1
end
end
end
end
13 changes: 13 additions & 0 deletions bundler/spec/dependabot/bundler/file_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,5 +724,18 @@
end
end
end

it "instruments the package manager version" do
events = []
Dependabot.subscribe(Dependabot::Notifications::FILE_PARSER_PACKAGE_MANAGER_VERSION_PARSED) do |*args|
events << ActiveSupport::Notifications::Event.new(*args)
end

parser.parse

expect(events.last.payload).to eq(
{ ecosystem: "bundler", package_managers: { "bundler" => "1" } }
)
end
end
end
1 change: 1 addition & 0 deletions common/dependabot-common.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Gem::Specification.new do |spec|
spec.required_ruby_version = ">= 2.5.0"
spec.required_rubygems_version = ">= 2.7.3"

spec.add_dependency "activesupport", ">= 6.0.0"
jurre marked this conversation as resolved.
Show resolved Hide resolved
spec.add_dependency "aws-sdk-codecommit", "~> 1.28"
spec.add_dependency "aws-sdk-ecr", "~> 1.5"
spec.add_dependency "bundler", ">= 1.16", "< 3.0.0"
Expand Down
2 changes: 2 additions & 0 deletions common/lib/dependabot/file_parsers/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# frozen_string_literal: true

require "dependabot/notifications"

module Dependabot
module FileParsers
class Base
Expand Down
17 changes: 17 additions & 0 deletions common/lib/dependabot/notifications.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# frozen_string_literal: true

require "active_support/notifications"

module Dependabot
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on keeping track of event names here as constants?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dig it!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it does make it harder to remove the instrumentation. When this is just a string, we can keep the subscribe call in the calling code while we remove all of the instrumentation code, the calling code will just noop. Now we need to be careful to not remove the constant because it would break upstream usage

Copy link
Member Author

Choose a reason for hiding this comment

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

In this single scenario I don't think ^ matters much, but when thinking about a generic system that lets us instrument all sort of things, I feel like optimizing to make it easy to add/delete notifications without friction is something to strive for. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, maybe doesn't make sense to add all notifications as constants but seems ok for ones that we care more about routing to specific places, for this one we'll probably want to end up persisting on update jobs directly whereas other ones we might just want to pass straight through to datadog?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that ideally it would be easier to add more, but I'm very mildly paranoid that core, as a public library, could have instrumentation added to it that we might not want to relay through our runner to instrumentation services.

Having a list of constants in Dependabot Core allows us to programmatically filter events as we can, by default relay all constants in Dependabot::Notifications.

We'd have to consciously bump dependabot-core in our runner to allow new notification to relay which would decouple a core upgrade from a sudden surge of new events hitting our instrumentation layer.

</tinfoil-hat>

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point @brrygrdn, my thinking was that we would only subscribe to specific events and not relay anything, but I can definitely see us moving towards that approach

module Notifications
FILE_PARSER_PACKAGE_MANAGER_VERSION_PARSED = "dependabot.file_parser.package_manager_version_parsed"
end

def self.instrument(name, payload = {})
jurre marked this conversation as resolved.
Show resolved Hide resolved
ActiveSupport::Notifications.instrument(name, payload)
end

def self.subscribe(pattern = nil, callback = nil, &block)
ActiveSupport::Notifications.subscribe(pattern, callback, &block)
end
end