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.

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

Add more information to the error message log #236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add more information to the error message log #236

wants to merge 2 commits into from

Conversation

glower
Copy link
Contributor

@glower glower commented Jun 13, 2019

  • Add detailed information about the error to the error response

@codecov-io
Copy link

codecov-io commented Jun 13, 2019

Codecov Report

Merging #236 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
+ Coverage   48.78%   48.93%   +0.14%     
==========================================
  Files          90       90              
  Lines        4747     4745       -2     
==========================================
+ Hits         2316     2322       +6     
+ Misses       2211     2206       -5     
+ Partials      220      217       -3
Impacted Files Coverage Δ
service/sign/handlers_sign.go 88.75% <100%> (ø) ⬆️
service/assertion/actions.go 86.9% <100%> (+5.72%) ⬆️
service/assertion/actions_user.go 73.61% <100%> (ø) ⬆️
service/pivot/handlers_pivot.go 82.56% <100%> (-0.16%) ⬇️
service/assertion/handlers_user.go 83.33% <0%> (+16.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be762b1...592ef7e. Read the comment docs.

@glower
Copy link
Contributor Author

glower commented Jun 13, 2019

This is a possible fix for #235
If this PR is OK I will migrate rest of the responses to have more information

@glower glower changed the title WIP: Add more information to the error message log Add more information to the error message log Jun 14, 2019
Copy link
Contributor

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

Generally snapd should do something semi-reasonable if error cases come as application/json with a "message" field. That seems to be the case here.

@@ -346,7 +346,8 @@ func findModel(brandID, modelName, serialNumer, apiKey string) (datastore.Model,
// Validate the model by checking that it exists on the database
model, err := datastore.Environ.DB.FindModel(brandID, modelName, apiKey)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

because of the try FindModel and then otherwise on failure eagerly try GetSubstoreModel it seems - as it was already - this will always produce slightly odd error messages/responses, this isn't new but if the goal is making for more understandable errors for the brand user this might need a larger rethink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will drop this PR and only put more info inside of the internal log. We can have a separate conversation about error responses.

Copy link
Contributor

@pedronis pedronis Jun 14, 2019

Choose a reason for hiding this comment

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

I would ask @cprov whether the change here is already enough of an improvement, he was the one having trouble debugging, before dropping them. For more clarity the issue here is how apiKey is checked implicitly in one case but not the other, I don't know the detail of the data model and other usages enough but in principle it would be better if the apiKey check was explicit/distinguishable unless we think leaking whether a model really exists or not is deemed a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree on that, but in this case, I just wanted to improve logging, not touching any business logic at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants