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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃搹 Standardize Error Keys #9251
Conversation
What would you say to making these constants in |
c28ddd7
to
135e1bb
Compare
@@ -63,7 +63,7 @@ def handle_unknown_error(err) | |||
"package-manager" => job.package_manager, |
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.
Thinking aloud: It's a little weird that everything else extracted from job.*
has a key of job-*
... but this doesn't. It's probably nbd but thought I'd mention it...
This should help those of us who don't always type so well 馃槄
cef4abe
to
a07aa22
Compare
4bec169
to
f15137f
Compare
6e19788
to
55db241
Compare
Sentry respects this, but we've not provided it.
1fb261c
to
f16011d
Compare
"job-dependency_group" => job.dependency_groups | ||
ErrorAttributes::CLASS => error.class.to_s, | ||
ErrorAttributes::MESSAGE => error.message, | ||
ErrorAttributes::BACKTRACE => error.backtrace.join("\n"), |
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.
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
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.
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.
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, 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?
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.
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 |
---|---|
We're using a mix of dashes and underscores, which I find confusing. I've introduced some failures trying to get these right, so I'm making all string keys dasherized.