Skip to content

[Updater] Prefer to use Dependabot::Service directly and as an application error reporter #6846

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

Merged
brrygrdn merged 10 commits into from
Mar 16, 2023
Merged

[Updater] Prefer to use Dependabot::Service directly and as an application error reporter #6846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bd37045
Remove Updater#errors, Service#errors is good enough
brrygrdn Mar 10, 2023
d710aa1
Remove the unusued Updater#error_context method
brrygrdn Mar 10, 2023
2f8103d
Fix privacy of the client setup
brrygrdn Mar 14, 2023
87d0c5e
Incorporate reporting to Sentry to the Dependabot::Service
brrygrdn Mar 15, 2023
d88d10c
Prefer service.record_update_job_error: update run halting errors
brrygrdn Mar 15, 2023
e3ba5e8
Prefer service.record_update_job_error: update specific error helper …
brrygrdn Mar 15, 2023
746bf38
Prefer service.record_update_job_error: update handle_dependabot_error
brrygrdn Mar 15, 2023
715daed
Prefer service.record_update_job_error: update handle_parse_error
brrygrdn Mar 15, 2023
298c6d0
Resolve some linting issues
brrygrdn Mar 15, 2023
8bdb6c6
Avoid nil assignment in case statement we need to check for
brrygrdn Mar 15, 2023
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
25 changes: 6 additions & 19 deletions updater/lib/dependabot/base_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,37 +64,24 @@ def handle_exception(err)
Dependabot.logger.error(err.message)
err.backtrace.each { |line| Dependabot.logger.error(line) }

Raven.capture_exception(err, raven_context)

service.capture_exception(error: err, job: job)
service.record_update_job_error(error_type: "unknown_error", error_details: { message: err.message })
end

def api_url
Environment.api_url
end

def job_id
Environment.job_id
end

def job_token
Environment.job_token
end

def api_client
@api_client ||= Dependabot::ApiClient.new(api_url, job_id, job_token)
@api_client ||= Dependabot::ApiClient.new(
Environment.api_url,
job_id,
Environment.job_token
)
end

def service
@service ||= Dependabot::Service.new(client: api_client)
end

private

def raven_context
context = { tags: {}, extra: { update_job_id: job_id } }
context[:tags][:package_manager] = job.package_manager if job
context
end
end
end
2 changes: 1 addition & 1 deletion updater/lib/dependabot/file_fetcher_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,8 @@ def handle_file_fetcher_error(error)
else
Dependabot.logger.error(error.message)
error.backtrace.each { |line| Dependabot.logger.error line }
Raven.capture_exception(error, raven_context)

service.capture_exception(error: error, job: job)
{ "error-type": "unknown_error" }
end

Expand Down
30 changes: 27 additions & 3 deletions updater/lib/dependabot/service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@
require "terminal-table"
require "dependabot/api_client"

# Wraps an API client with the current state of communications with the Dependabot Service
# and provides an interface to summarise all actions taken.
# This class provides an output adapter for the Dependabot Service which manages
# communication with the private API as well as consolidated error handling.
#
# Currently this is the only output adapter available, but in future we may
# support others for use with the dependabot/cli project.
#
module Dependabot
class Service
extend Forwardable
attr_reader :client, :events, :pull_requests, :errors
attr_reader :pull_requests, :errors

def initialize(client:)
@client = client
Expand Down Expand Up @@ -39,6 +42,25 @@ def record_update_job_error(error_type:, error_details:, dependency: nil)
client.record_update_job_error(error_type: error_type, error_details: error_details)
end

# This method wraps the Raven client as the Application error tracker
# the service uses to notice errors.
#
# This should be called as an alternative/in addition to record_update_job_error
# for cases where an error could indicate a problem with the service.
def capture_exception(error:, job: nil, dependency: nil, tags: {}, extra: {})
Raven.capture_exception(
error,
{
tags: tags,
extra: extra.merge({
update_job_id: job&.id,
package_manager: job&.package_manager,
dependency_name: dependency&.name
}.compact)
Comment on lines +55 to +59
Copy link
Copy Markdown
Contributor

@landongrindheim landongrindheim Mar 15, 2023

Choose a reason for hiding this comment

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

🔎 I understand the impulse to send only those key/value pairs for which we have data, but I wonder if the absence of values is valuable for understanding context.

As a 🤏 cheap argument, I'd be able to glean much more insight from:

{
  update_job_id: 1,
  dependency_name: "dependabot-fortran",
  package_manager: nil
}

than

{
  update_job_id: 1,
  dependency_name: "dependabot-fortran"
}

Copy link
Copy Markdown
Contributor Author

@brrygrdn brrygrdn Mar 15, 2023

Choose a reason for hiding this comment

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

That's fair, I think we should actually expect dependency_name to be passed/present move often -but- I'm err'ing on the side of maintaining existing behaviour for now so I'd prefer to pass on this to avoid test churn for now.

}
)
end

def noop?
pull_requests.empty? && errors.empty?
end
Expand Down Expand Up @@ -71,6 +93,8 @@ def summary

private

attr_reader :client

def pull_request_summary
return unless pull_requests.any?

Expand Down
Loading