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

Send Ecosystem Metrics to Dependabot-API on Update Job Completion #10905

Merged
merged 3 commits into from
Nov 13, 2024
Merged

Send Ecosystem Metrics to Dependabot-API on Update Job Completion #10905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67643bf
send ecosystem metrics to dependabot-api
kbukum1 Nov 12, 2024
5d72088
add sorbet nil check
kbukum1 Nov 12, 2024
3ee82ca
added spec when ecosystem is nil for `record_ecosystem_meta`
kbukum1 Nov 13, 2024
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
2 changes: 1 addition & 1 deletion common/lib/dependabot/requirement.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def constraints
end

# Returns the highest lower limit among all minimum constraints.
sig { returns(T.nilable(Gem::Version)) }
sig { returns(T.nilable(Dependabot::Version)) }
def min_version
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review Tip: This PR addresses type issues introduced in the previous PR by ensuring all versions consistently use Dependabot::Version instead of the more generic Gem::Version. This change provides more specific typing, improving clarity and reducing abstraction.

# Select constraints with minimum operators
min_constraints = requirements.select { |op, _| MINIMUM_OPERATORS.include?(op) }
Expand Down
98 changes: 89 additions & 9 deletions updater/lib/dependabot/api_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ class ApiError < StandardError; end
class ApiClient
extend T::Sig

MAX_REQUEST_RETRIES = 3

sig { params(base_url: String, job_id: T.any(String, Integer), job_token: String).void }
def initialize(base_url, job_id, job_token)
@base_url = base_url
Expand All @@ -43,7 +45,7 @@ def create_pull_request(dependency_change, base_commit_sha)
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
raise if retry_count > 3
raise if retry_count > MAX_REQUEST_RETRIES

sleep(rand(3.0..10.0))
retry
Expand Down Expand Up @@ -72,7 +74,7 @@ def update_pull_request(dependency_change, base_commit_sha)
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
raise if retry_count > 3
raise if retry_count > MAX_REQUEST_RETRIES

sleep(rand(3.0..10.0))
retry
Expand All @@ -92,7 +94,7 @@ def close_pull_request(dependency_names, reason)
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
raise if retry_count > 3
raise if retry_count > MAX_REQUEST_RETRIES

sleep(rand(3.0..10.0))
retry
Expand All @@ -119,7 +121,7 @@ def record_update_job_error(error_type:, error_details:)
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
raise if retry_count > 3
raise if retry_count > MAX_REQUEST_RETRIES

sleep(rand(3.0..10.0))
retry
Expand Down Expand Up @@ -154,7 +156,7 @@ def record_update_job_warning(warn_type:, warn_title:, warn_description:)
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
raise if retry_count > 3
raise if retry_count > MAX_REQUEST_RETRIES

sleep(rand(3.0..10.0))
retry
Expand All @@ -180,7 +182,7 @@ def record_update_job_unknown_error(error_type:, error_details:)
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
raise if retry_count > 3
raise if retry_count > MAX_REQUEST_RETRIES

sleep(rand(3.0..10.0))
retry
Expand All @@ -200,7 +202,7 @@ def mark_job_as_processed(base_commit_sha)
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
raise if retry_count > 3
raise if retry_count > MAX_REQUEST_RETRIES

sleep(rand(3.0..10.0))
retry
Expand All @@ -224,7 +226,7 @@ def update_dependency_list(dependencies, dependency_files)
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
raise if retry_count > 3
raise if retry_count > MAX_REQUEST_RETRIES

sleep(rand(3.0..10.0))
retry
Expand All @@ -243,7 +245,7 @@ def record_ecosystem_versions(ecosystem_versions)
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError
retry_count ||= 0
retry_count += 1
raise if retry_count > 3
raise if retry_count > MAX_REQUEST_RETRIES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review Tip: Moved retry request count in constant variable since it is always equal 3.

sleep(rand(3.0..10.0))
retry
Expand Down Expand Up @@ -274,8 +276,86 @@ def increment_metric(metric, tags:)
end
end

sig { params(ecosystem: T.nilable(Ecosystem)).void }
def record_ecosystem_meta(ecosystem)
return unless Dependabot::Experiments.enabled?(:enable_record_ecosystem_meta)

return if ecosystem.nil?

begin
::Dependabot::OpenTelemetry.tracer.in_span("record_ecosystem_meta", kind: :internal) do |_span|
api_url = "#{base_url}/update_jobs/#{job_id}/record_ecosystem_meta"

body = {
data: [
{
ecosystem: {
name: ecosystem.name,
package_manager: version_manager_json(ecosystem.package_manager),
language: version_manager_json(ecosystem.language)
}
}
]
}

retry_count = 0

begin
response = http_client.post(api_url, json: body)
raise ApiError, response.body if response.code >= 400
rescue HTTP::ConnectionError, OpenSSL::SSL::SSLError, ApiError => e
retry_count += 1
if retry_count <= MAX_REQUEST_RETRIES
sleep(rand(3.0..10.0))
retry
else
Dependabot.logger.error(
"Failed to record ecosystem meta after #{MAX_REQUEST_RETRIES} retries: #{e.message}"
)
end
end
end
rescue StandardError => e
Dependabot.logger.error("Failed to record ecosystem meta: #{e.message}")
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review Tip: Create metrics from ecosystem instance that stores ecosystem, package manager and language information.

private

# Update return type to allow returning a Hash or nil
sig do
params(version_manager: T.nilable(Dependabot::Ecosystem::VersionManager))
.returns(T.nilable(T::Hash[String, T.untyped]))
end
def version_manager_json(version_manager)
return nil unless version_manager

{
name: version_manager.name,
raw_version: version_manager.version.to_semver.to_s,
version: version_manager.version.to_s,
requirement: version_manager_requirement_json(version_manager)
}
end

# Update return type to allow returning a Hash or nil
sig do
params(version_manager: Dependabot::Ecosystem::VersionManager)
.returns(T.nilable(T::Hash[String, T.untyped]))
end
def version_manager_requirement_json(version_manager)
requirement = version_manager.requirement
return nil unless requirement

{
raw_constraint: requirement.constraints.join(", "),
min_raw_version: requirement.min_version&.to_semver.to_s,
min_version: requirement.min_version&.to_s,
max_raw_version: requirement.max_version&.to_semver.to_s,
max_version: requirement.max_version&.to_s
}
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review Tip: Use common metrics creation functions for package manager and language since they have metrics in same structure.

sig { returns(String) }
attr_reader :base_url

Expand Down
3 changes: 2 additions & 1 deletion updater/lib/dependabot/service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ def initialize(client:)
def_delegators :client,
:mark_job_as_processed,
:record_ecosystem_versions,
:increment_metric
:increment_metric,
:record_ecosystem_meta

sig { void }
def wait_for_calls_to_finish
Expand Down
3 changes: 3 additions & 0 deletions updater/lib/dependabot/updater/group_update_refreshing.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ def upsert_pull_request_with_error_handling(dependency_change, group)
end
rescue StandardError => e
error_handler.handle_job_error(error: e, dependency_group: dependency_snapshot.job_group)
ensure
# record metrics for the ecosystem
service.record_ecosystem_meta(dependency_snapshot.ecosystem)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review Tip: Send metrics for group refresh operation after update process (success, or fail)

kbukum1 marked this conversation as resolved.
Show resolved Hide resolved

# Having created the dependency_change, we need to determine the right strategy to apply it to the project:
Expand Down
2 changes: 2 additions & 0 deletions updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ def perform
service.create_pull_request(T.must(dependency_change), dependency_snapshot.base_commit_sha)
rescue StandardError => e
error_handler.handle_job_error(error: e, dependency_group: group)
ensure
service.record_ecosystem_meta(dependency_snapshot.ecosystem)
end
Copy link
Contributor Author

@kbukum1 kbukum1 Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Review Tip: Send metrics for create group operation after update process

else
Dependabot.logger.info("Nothing to update for Dependency Group: '#{group.name}'")
Expand Down
2 changes: 2 additions & 0 deletions updater/lib/dependabot/updater/operations/create_security_update_pull_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ def check_and_create_pr_with_error_handling(dependency)
)
rescue StandardError => e
error_handler.handle_dependency_error(error: e, dependency: dependency)
ensure
service.record_ecosystem_meta(dependency_snapshot.ecosystem)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Review Tip: Send metrics for create security update after update process (success, or fail). Just being sure that metrics is getting called when there is update. We don't prefer to send metrics when there is not update necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

@kbukum1: Not logging no update necessary events simplifies logging but I would suggest, we should log no update as well. The reason being without logging no update for Dependabot job run, makes it hard to gauge the baseline activity level (example: lose the ability to compare how often checks lead to updates versus when no updates are necessary). Once we have this data we can reason with it. Same will be true at different places where you are sending metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abdulapopoola what do you think. Should we log all update processes? This may be easier to implement but for each update process we are going to send metrics even through there may be no update

Copy link
Contributor Author

@kbukum1 kbukum1 Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@honeyankit ,

Just wanted to mention, normally we discussed that when we were preparing the ADR and we thought it will be ok to start with only if there is update. We were thinking to add no-update later if we see is necessary.

Copy link
Contributor Author

@kbukum1 kbukum1 Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@honeyankit ,

It is fixed now. I am sending metrics if update process started for a dependency or a group even there is no update.


# rubocop:disable Metrics/AbcSize
Expand Down
3 changes: 3 additions & 0 deletions updater/lib/dependabot/updater/operations/refresh_security_update_pull_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ def perform
check_and_update_pull_request(dependencies)
rescue StandardError => e
error_handler.handle_dependency_error(error: e, dependency: dependencies.last)
ensure
# Record ecosystem metrics for the update job
service.record_ecosystem_meta(dependency_snapshot.ecosystem)
end

private
Expand Down
2 changes: 2 additions & 0 deletions updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ def perform
check_and_update_pull_request(dependencies)
rescue StandardError => e
error_handler.handle_dependency_error(error: e, dependency: dependency)
ensure
service.record_ecosystem_meta(dependency_snapshot.ecosystem)
end

private
Expand Down
8 changes: 6 additions & 2 deletions updater/lib/dependabot/updater/operations/update_all_versions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,10 @@ def dependencies
def check_and_create_pr_with_error_handling(dependency)
check_and_create_pull_request(dependency)
rescue URI::InvalidURIError => e
error_handler.handle_dependency_error(error: Dependabot::DependencyFileNotResolvable.new(e.message),
dependency: dependency)
error_handler.handle_dependency_error(
error: Dependabot::DependencyFileNotResolvable.new(e.message),
dependency: dependency
)
rescue Dependabot::InconsistentRegistryResponse => e
error_handler.log_dependency_error(
dependency: dependency,
Expand All @@ -104,6 +106,8 @@ def check_and_create_pr_with_error_handling(dependency)
)
rescue StandardError => e
process_dependency_error(e, dependency)
ensure
service.record_ecosystem_meta(dependency_snapshot.ecosystem)
end
Copy link
Contributor Author

@kbukum1 kbukum1 Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Review Tip: Send metrics for update all after update process. Just being sure that metrics is getting called when there is update. We don't prefer to send metrics when there is not update necessary


# rubocop:disable Metrics/AbcSize
Expand Down
85 changes: 84 additions & 1 deletion updater/spec/dependabot/api_client_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@

before do
allow(Dependabot::PullRequestCreator::MessageBuilder).to receive_message_chain(:new, :message).and_return(message)

allow(Dependabot::Experiments).to receive(:enabled?).with(:enable_record_ecosystem_meta).and_return(true)
stub_request(:post, create_pull_request_url)
.to_return(status: 204, headers: headers)
end
Expand Down Expand Up @@ -509,4 +509,87 @@
end
end
end

describe "record_ecosystem_meta" do
before do
allow(Dependabot::Experiments).to receive(:enabled?).with(:enable_record_ecosystem_meta).and_return(true)
end

let(:ecosystem) do
Dependabot::Ecosystem.new(
name: "bundler",
package_manager: instance_double(
Dependabot::Ecosystem::VersionManager,
name: "bundler",
version: Dependabot::Version.new("2.1.4"),
requirement: instance_double(
Dependabot::Requirement,
constraints: [">= 2.0"],
min_version: Dependabot::Version.new("2.0.0"),
max_version: Dependabot::Version.new("3.0.0")
)
),
language: instance_double(
Dependabot::Ecosystem::VersionManager,
name: "ruby",
version: Dependabot::Version.new("2.7.0"),
requirement: nil
)
)
end
let(:record_ecosystem_meta_url) { "http://example.com/update_jobs/1/record_ecosystem_meta" }

it "hits the correct endpoint" do
client.record_ecosystem_meta(ecosystem)

expect(WebMock)
.to have_requested(:post, record_ecosystem_meta_url)
.with(headers: { "Authorization" => "token" })
end

it "encodes the payload correctly" do
client.record_ecosystem_meta(ecosystem)

expect(WebMock).to(have_requested(:post, record_ecosystem_meta_url).with do |req|
data = JSON.parse(req.body)["data"][0]["ecosystem"]

expect(data).not_to be_nil # Ensure data is present
expect(data["name"]).to eq("bundler")
expect(data["package_manager"]).to include(
"name" => "bundler",
"raw_version" => "2.1.4",
"version" => "2.1.4",
"requirement" => {
"max_raw_version" => "3.0.0",
"max_version" => "3.0.0",
"min_raw_version" => "2.0.0",
"min_version" => "2.0.0",
"raw_constraint" => ">= 2.0"
}
)
expect(data["language"]).to include(
"name" => "ruby",
"version" => "2.7.0"
)
end)
end

context "when ecosystem is nil" do
it "does not send a request" do
client.record_ecosystem_meta(nil)
expect(WebMock).not_to have_requested(:post, record_ecosystem_meta_url)
end
end

context "when feature flag is disabled" do
before do
allow(Dependabot::Experiments).to receive(:enabled?).with(:enable_record_ecosystem_meta).and_return(false)
end

it "does not send a request" do
client.record_ecosystem_meta(ecosystem)
expect(WebMock).not_to have_requested(:post, record_ecosystem_meta_url)
end
end
end
end
9 changes: 8 additions & 1 deletion updater/spec/dependabot/updater/operations/create_group_update_pull_request_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,14 @@
end

let(:mock_service) do
instance_double(Dependabot::Service, create_pull_request: nil, update_pull_request: nil, close_pull_request: nil)
instance_double(
Dependabot::Service,
increment_metric: nil,
record_update_job_error: nil,
create_pull_request: nil,
record_update_job_warning: nil,
record_ecosystem_meta: nil
)
end
let(:mock_error_handler) { instance_double(Dependabot::Updater::ErrorHandler) }

Expand Down
3 changes: 2 additions & 1 deletion updater/spec/dependabot/updater/operations/create_security_update_pull_request_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
increment_metric: nil,
record_update_job_error: nil,
create_pull_request: nil,
record_update_job_warning: nil
record_update_job_warning: nil,
record_ecosystem_meta: nil
)
end

Expand Down
Loading
Loading