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

Jump to bottom

📏 Standardize Error Keys #9251

Merged
merged 8 commits into from
Mar 13, 2024
Merged

📏 Standardize Error Keys #9251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
135e1bb
Standardize error keys
landongrindheim Mar 8, 2024
a07aa22
Use constants to keep keys consistent
landongrindheim Mar 12, 2024
372abcc
Use GROUPS (plural) for consistency
landongrindheim Mar 12, 2024
5cb28bf
Send `fingerprint` to distinguish errors
landongrindheim Mar 12, 2024
f16011d
Rely on `T.unsafe` to access `#sentry_context`
landongrindheim Mar 12, 2024
c9a5812
Merge branch 'main' into standardize-error-reporting-keys
landongrindheim Mar 13, 2024
56abecf
Merge branch 'main' into standardize-error-reporting-keys
landongrindheim Mar 13, 2024
d34dfb6
Merge branch 'main' into standardize-error-reporting-keys
landongrindheim Mar 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
12 changes: 12 additions & 0 deletions common/lib/dependabot/errors.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,18 @@
module Dependabot
extend T::Sig

module ErrorAttributes
BACKTRACE = "error-backtrace"
CLASS = "error-class"
DETAILS = "error-details"
FINGERPRINT = "fingerprint"
MESSAGE = "error-message"
DEPENDENCIES = "job-dependencies"
DEPENDENCY_GROUPS = "job-dependency-groups"
JOB_ID = "job-id"
PACKAGE_MANAGER = "package-manager"
end

# rubocop:disable Metrics/MethodLength
sig { params(error: StandardError).returns(T.nilable(T::Hash[Symbol, T.untyped])) }
def self.fetcher_error_details(error)
Expand Down
16 changes: 9 additions & 7 deletions updater/lib/dependabot/base_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# frozen_string_literal: true

require "dependabot/api_client"
require "dependabot/errors"
require "dependabot/service"
require "dependabot/logger"
require "dependabot/logger/formats"
Expand Down Expand Up @@ -57,13 +58,14 @@ def handle_exception(err)

def handle_unknown_error(err)
error_details = {
"error-class" => err.class.to_s,
"error-message" => err.message,
"error-backtrace" => err.backtrace.join("\n"),
"package-manager" => job.package_manager,
"job-id" => job.id,
"job-dependencies" => job.dependencies,
"job-dependency_group" => job.dependency_groups
ErrorAttributes::CLASS => err.class.to_s,
ErrorAttributes::MESSAGE => err.message,
ErrorAttributes::BACKTRACE => err.backtrace.join("\n"),
ErrorAttributes::FINGERPRINT => err.respond_to?(:sentry_context) ? err.sentry_context[:fingerprint] : nil,
ErrorAttributes::PACKAGE_MANAGER => job.package_manager,
ErrorAttributes::JOB_ID => job.id,
ErrorAttributes::DEPENDENCIES => job.dependencies,
ErrorAttributes::DEPENDENCY_GROUPS => job.dependency_groups
}.compact
service.record_update_job_unknown_error(error_type: "updater_error", error_details: error_details)
service.increment_metric("updater.update_job_unknown_error", tags: {
Expand Down
16 changes: 9 additions & 7 deletions updater/lib/dependabot/file_fetcher_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

require "base64"
require "dependabot/base_command"
require "dependabot/errors"
require "dependabot/opentelemetry"
require "dependabot/updater"
require "octokit"
Expand Down Expand Up @@ -173,13 +174,14 @@ def handle_file_fetcher_error(error)
log_error(error)

unknown_error_details = {
"error-class" => error.class.to_s,
"error-message" => error.message,
"error-backtrace" => error.backtrace.join("\n"),
"package-manager" => job.package_manager,
"job-id" => job.id,
"job-dependencies" => job.dependencies,
"job-dependency_group" => job.dependency_groups
ErrorAttributes::CLASS => error.class.to_s,
ErrorAttributes::MESSAGE => error.message,
ErrorAttributes::BACKTRACE => error.backtrace.join("\n"),
Copy link
Member

@abdulapopoola abdulapopoola Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

along the lines of creating the ERROR_ATTRIBUTES constant; would it be useful to create a method to handle generating the unknown_error_details which accepts and error and job parameters? That would facilitate reuse and standardization

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 considered this, but didn't pursue it because Service#capture_exception accepts other arguments for job-dependencies and job-dependency-groups. These are being provided in ErrorHandler#error_details_for. IMO, it'd be preferable to standardize usage, but I think that stretches the task I'm trying to complete a little too far.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, please for my learning; from this code, the dependency and the dependency-groups seem to come from the same job value. If so, what are the tradeoffs being considered and the costs involved?

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'm not 100% sure I understand the question, but I'll speak to what I think I understand.

Within Service#capture_exception, we prioritize the values passed in as dependency and dependency_group (ex dependency&.name || job&.dependencies). There's one place where these values are being provided 👇

          service.capture_exception(
            error: error,
            job: job,
            dependency: dependency,
            dependency_group: dependency_group
          )

This method is being delegated to by other methods, which appear at pretty high levels 👇 I count seven places that would need to be updated/refactored to make this a re-usable method that only fetches the information from job. I don't love the current state, but I do see the value that's gained by getting reports about individual dependencies (vs all dependencies for the job).

#handle_job_error #handle_dependency_error
image image

ErrorAttributes::FINGERPRINT => error.respond_to?(:sentry_context) ? error.sentry_context[:fingerprint] : nil,
ErrorAttributes::PACKAGE_MANAGER => job.package_manager,
ErrorAttributes::JOB_ID => job.id,
ErrorAttributes::DEPENDENCIES => job.dependencies,
ErrorAttributes::DEPENDENCY_GROUPS => job.dependency_groups
}.compact

error_details = {
Expand Down
16 changes: 9 additions & 7 deletions updater/lib/dependabot/service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
require "terminal-table"

require "dependabot/api_client"
require "dependabot/errors"
require "dependabot/opentelemetry"

# This class provides an output adapter for the Dependabot Service which manages
Expand Down Expand Up @@ -105,13 +106,14 @@ def capture_exception(error:, job: nil, dependency: nil, dependency_group: nil,
return unless Experiments.enabled?(:record_update_job_unknown_error)

error_details = {
"error-class" => error.class.to_s,
"error-message" => error.message,
"error-backtrace" => error.backtrace&.join("\n"),
"package-manager" => job&.package_manager,
"job-id" => job&.id,
"job-dependencies" => dependency&.name || job&.dependencies,
"job-dependency-group" => dependency_group&.name || job&.dependency_groups
ErrorAttributes::CLASS => error.class.to_s,
ErrorAttributes::MESSAGE => error.message,
ErrorAttributes::BACKTRACE => error.backtrace&.join("\n"),
ErrorAttributes::FINGERPRINT => error.respond_to?(:sentry_context) ? T.unsafe(error).sentry_context[:fingerprint] : nil, # rubocop:disable Layout/LineLength
ErrorAttributes::PACKAGE_MANAGER => job&.package_manager,
ErrorAttributes::JOB_ID => job&.id,
ErrorAttributes::DEPENDENCIES => dependency&.name || job&.dependencies,
ErrorAttributes::DEPENDENCY_GROUPS => dependency_group&.name || job&.dependency_groups
}.compact
record_update_job_unknown_error(error_type: "unknown_error", error_details: error_details)
end
Expand Down
20 changes: 11 additions & 9 deletions updater/lib/dependabot/update_files_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
require "base64"
require "dependabot/base_command"
require "dependabot/dependency_snapshot"
require "dependabot/errors"
require "dependabot/opentelemetry"
require "dependabot/updater"

Expand Down Expand Up @@ -63,7 +64,7 @@ def base_commit_sha
Environment.job_definition["base_commit_sha"]
end

# rubocop:disable Metrics/AbcSize
# rubocop:disable Metrics/AbcSize, Layout/LineLength
def handle_parser_error(error)
# This happens if the repo gets removed after a job gets kicked off.
# The service will handle the removal without any prompt from the updater,
Expand All @@ -82,13 +83,14 @@ def handle_parser_error(error)
Dependabot.logger.error error.message
error.backtrace.each { |line| Dependabot.logger.error line }
unknown_error_details = {
"error-class" => error.class.to_s,
"error-message" => error.message,
"error-backtrace" => error.backtrace.join("\n"),
"package-manager" => job.package_manager,
"job-id" => job.id,
"job-dependencies" => job.dependencies,
"job-dependency_group" => job.dependency_groups
ErrorAttributes::CLASS => error.class.to_s,
ErrorAttributes::MESSAGE => error.message,
ErrorAttributes::BACKTRACE => error.backtrace.join("\n"),
ErrorAttributes::FINGERPRINT => error.respond_to?(:sentry_context) ? error.sentry_context[:fingerprint] : nil,
ErrorAttributes::PACKAGE_MANAGER => job.package_manager,
ErrorAttributes::JOB_ID => job.id,
ErrorAttributes::DEPENDENCIES => job.dependencies,
ErrorAttributes::DEPENDENCY_GROUPS => job.dependency_groups
}.compact

service.capture_exception(error: error, job: job)
Expand All @@ -113,6 +115,6 @@ def handle_parser_error(error)
error_details: error_details[:"error-detail"]
)
end
# rubocop:enable Metrics/AbcSize
# rubocop:enable Metrics/AbcSize, Layout/LineLength
end
end
15 changes: 8 additions & 7 deletions updater/lib/dependabot/updater/error_handler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,14 @@ def error_details_for(error, dependency: nil, dependency_group: nil)

def log_unknown_error_with_backtrace(error)
error_details = {
"error-class" => error.class.to_s,
"error-message" => error.message,
"error-backtrace" => error.backtrace.join("\n"),
"package-manager" => job.package_manager,
"job-id" => job.id,
"job-dependencies" => job.dependencies,
"job-dependency_group" => job.dependency_groups
ErrorAttributes::CLASS => error.class.to_s,
ErrorAttributes::MESSAGE => error.message,
ErrorAttributes::BACKTRACE => error.backtrace.join("\n"),
ErrorAttributes::FINGERPRINT => error.respond_to?(:sentry_context) ? error.sentry_context[:fingerprint] : nil,
ErrorAttributes::PACKAGE_MANAGER => job.package_manager,
ErrorAttributes::JOB_ID => job.id,
ErrorAttributes::DEPENDENCIES => job.dependencies,
ErrorAttributes::DEPENDENCY_GROUPS => job.dependency_groups
}.compact

service.increment_metric("updater.update_job_unknown_error", tags: {
Expand Down
37 changes: 19 additions & 18 deletions updater/spec/dependabot/file_fetcher_command_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

require "spec_helper"
require "dependabot/file_fetcher_command"
require "dependabot/errors"
require "tmpdir"

require "support/dummy_package_manager/dummy"
Expand Down Expand Up @@ -139,12 +140,12 @@
expect(api_client).to receive(:record_update_job_error).with(
error_type: "file_fetcher_error",
error_details: {
"error-backtrace" => an_instance_of(String),
"error-message" => "my_branch",
"error-class" => "StandardError",
"package-manager" => "bundler",
"job-id" => "123123",
"job-dependency_group" => []
Dependabot::ErrorAttributes::BACKTRACE => an_instance_of(String),
Dependabot::ErrorAttributes::MESSAGE => "my_branch",
Dependabot::ErrorAttributes::CLASS => "StandardError",
Dependabot::ErrorAttributes::PACKAGE_MANAGER => "bundler",
Dependabot::ErrorAttributes::JOB_ID => "123123",
Dependabot::ErrorAttributes::DEPENDENCY_GROUPS => []
}
)
expect(api_client).to receive(:record_update_job_unknown_error)
Expand All @@ -157,12 +158,12 @@
expect(api_client).to receive(:record_update_job_unknown_error).with(
error_type: "unknown_error",
error_details: {
"error-backtrace" => an_instance_of(String),
"error-message" => "my_branch",
"error-class" => "StandardError",
"package-manager" => "bundler",
"job-id" => "123123",
"job-dependency-group" => []
Dependabot::ErrorAttributes::BACKTRACE => an_instance_of(String),
Dependabot::ErrorAttributes::MESSAGE => "my_branch",
Dependabot::ErrorAttributes::CLASS => "StandardError",
Dependabot::ErrorAttributes::PACKAGE_MANAGER => "bundler",
Dependabot::ErrorAttributes::JOB_ID => "123123",
Dependabot::ErrorAttributes::DEPENDENCY_GROUPS => []
}
)
expect(api_client).to receive(:mark_job_as_processed)
Expand All @@ -182,12 +183,12 @@
expect(api_client).to receive(:record_update_job_error).with(
error_type: "file_fetcher_error",
error_details: {
"error-backtrace" => an_instance_of(String),
"error-message" => "my_branch",
"error-class" => "StandardError",
"package-manager" => "bundler",
"job-id" => "123123",
"job-dependency_group" => []
Dependabot::ErrorAttributes::BACKTRACE => an_instance_of(String),
Dependabot::ErrorAttributes::MESSAGE => "my_branch",
Dependabot::ErrorAttributes::CLASS => "StandardError",
Dependabot::ErrorAttributes::PACKAGE_MANAGER => "bundler",
Dependabot::ErrorAttributes::JOB_ID => "123123",
Dependabot::ErrorAttributes::DEPENDENCY_GROUPS => []
}
)
expect(api_client).to receive(:mark_job_as_processed)
Expand Down
24 changes: 12 additions & 12 deletions updater/spec/dependabot/service_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,8 @@
.with(
error_type: "unknown_error",
error_details: hash_including(
"error-message" => "Something went wrong",
"error-class" => "Dependabot::DependabotError"
Dependabot::ErrorAttributes::MESSAGE => "Something went wrong",
Dependabot::ErrorAttributes::CLASS => "Dependabot::DependabotError"
)
)
end
Expand All @@ -317,10 +317,10 @@
.with(
error_type: "unknown_error",
error_details: hash_including(
"error-class" => "Dependabot::DependabotError",
"error-message" => "Something went wrong",
"job-id" => job.id,
"package-manager" => job.package_manager
Dependabot::ErrorAttributes::CLASS => "Dependabot::DependabotError",
Dependabot::ErrorAttributes::MESSAGE => "Something went wrong",
Dependabot::ErrorAttributes::JOB_ID => job.id,
Dependabot::ErrorAttributes::PACKAGE_MANAGER => job.package_manager
)
)
end
Expand All @@ -334,9 +334,9 @@
.with(
error_type: "unknown_error",
error_details: hash_including(
"error-message" => "Something went wrong",
"error-class" => "Dependabot::DependabotError",
"job-dependencies" => "lodash"
Dependabot::ErrorAttributes::MESSAGE => "Something went wrong",
Dependabot::ErrorAttributes::CLASS => "Dependabot::DependabotError",
Dependabot::ErrorAttributes::DEPENDENCIES => "lodash"
)
)
end
Expand All @@ -351,9 +351,9 @@
.with(
error_type: "unknown_error",
error_details: hash_including(
"error-message" => "Something went wrong",
"error-class" => "Dependabot::DependabotError",
"job-dependency-group" => "all-the-things"
Dependabot::ErrorAttributes::MESSAGE => "Something went wrong",
Dependabot::ErrorAttributes::CLASS => "Dependabot::DependabotError",
Dependabot::ErrorAttributes::DEPENDENCY_GROUPS => "all-the-things"
)
)
end
Expand Down
36 changes: 18 additions & 18 deletions updater/spec/dependabot/update_files_command_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,12 @@
expect(service).to receive(:record_update_job_error).with(
error_type: "update_files_error",
error_details: {
"error-backtrace" => an_instance_of(String),
"error-message" => "hell",
"error-class" => "StandardError",
"package-manager" => "bundler",
"job-id" => "123123",
"job-dependency_group" => []
Dependabot::ErrorAttributes::BACKTRACE => an_instance_of(String),
Dependabot::ErrorAttributes::MESSAGE => "hell",
Dependabot::ErrorAttributes::CLASS => "StandardError",
Dependabot::ErrorAttributes::PACKAGE_MANAGER => "bundler",
Dependabot::ErrorAttributes::JOB_ID => "123123",
Dependabot::ErrorAttributes::DEPENDENCY_GROUPS => []
}
)

Expand All @@ -147,12 +147,12 @@
expect(service).to receive(:record_update_job_unknown_error).with(
error_type: "update_files_error",
error_details: {
"error-backtrace" => an_instance_of(String),
"error-message" => "hell",
"error-class" => "StandardError",
"package-manager" => "bundler",
"job-id" => "123123",
"job-dependency_group" => []
Dependabot::ErrorAttributes::BACKTRACE => an_instance_of(String),
Dependabot::ErrorAttributes::MESSAGE => "hell",
Dependabot::ErrorAttributes::CLASS => "StandardError",
Dependabot::ErrorAttributes::PACKAGE_MANAGER => "bundler",
Dependabot::ErrorAttributes::JOB_ID => "123123",
Dependabot::ErrorAttributes::DEPENDENCY_GROUPS => []
}
)

Expand All @@ -171,12 +171,12 @@
expect(service).to receive(:record_update_job_error).with(
error_type: "update_files_error",
error_details: {
"error-backtrace" => an_instance_of(String),
"error-message" => "hell",
"error-class" => "StandardError",
"package-manager" => "bundler",
"job-id" => "123123",
"job-dependency_group" => []
Dependabot::ErrorAttributes::BACKTRACE => an_instance_of(String),
Dependabot::ErrorAttributes::MESSAGE => "hell",
Dependabot::ErrorAttributes::CLASS => "StandardError",
Dependabot::ErrorAttributes::PACKAGE_MANAGER => "bundler",
Dependabot::ErrorAttributes::JOB_ID => "123123",
Dependabot::ErrorAttributes::DEPENDENCY_GROUPS => []
}
)

Expand Down
Loading
Loading