API error improvements #3091

dbutenhof · 2022-11-30T21:06:22Z

PBENCH-995

The dashboard is sometimes receiving HTTP errors without {"message"} JSON payloads. It's not entirely clear why this is happening given the APIAbort logic, but we could provide more useful hints in most cases and I've tried to "fill in the blanks" in places where we'd omitted the message parameter.

In the course of this, I also added logic (mimicking that in several other places) to aggregate errors when using PUT /datasets/metadata to set multiple keys, when a weird error occurs in just a few. I realized that this error path wasn't tested, and added a unit test.

lib/pbench/server/api/resources/query_apis/__init__.py

lib/pbench/server/api/resources/query_apis/datasets/datasets_mappings.py

lib/pbench/server/api/resources/query_apis/datasets/__init__.py

lib/pbench/test/unit/server/query_apis/test_datasets_mappings.py

portante

Do we really want to expose internal state in the error message?

Perhaps we could use a UUID of some sort in the payload of the HTTPS response message payload, that one could search for in the logs of the server to help debugging.

npalaska

Looks good but black is unhappy with your query_apis/__ini__.py file

dbutenhof · 2022-12-01T12:54:49Z

Perhaps we could use a UUID of some sort in the payload of the HTTPS response message payload, that one could search for in the logs of the server to help debugging.

While I don't think this is directly related, we would like to make it easier to find the stream of log entries leading up to an error in the log, and we have a Jira ticket that goes way back to a PR comment discussion "ages" ago. That's PBENCH-459... but this PR is a separate and more limited effort.

siddardh-ra

Looks good 👍

webbnh

Looks like some nice improvements, but I found a few things you might want to tweak.

lib/pbench/server/api/resources/query_apis/__init__.py

lib/pbench/server/api/resources/query_apis/datasets/__init__.py

lib/pbench/server/api/resources/query_apis/__init__.py

lib/pbench/test/unit/server/test_datasets_metadata.py

lib/pbench/server/api/resources/datasets_metadata.py

lib/pbench/test/unit/server/test_datasets_metadata.py

webbnh

It seems good. I have a concern about test_put_set_errors(), which I hope you can allay, and a request for it as well, but it doesn't have to gate the merge.

lib/pbench/test/unit/server/test_datasets_metadata.py

npalaska

Looks good

lib/pbench/server/api/resources/query_apis/datasets/__init__.py

lib/pbench/server/api/resources/datasets_metadata.py

webbnh

This looks like a great step forward, but I think it would be better if APIInternalError set the exception message to a boilerplate (including the UUID) and sent the input parameter message to the log. I think that that will simplify the places where it is used (avoiding double logging), and it will free the caller of having to sanitize the error message (which we've already concluded shouldn't convey much more than the UUID anyway).

lib/pbench/server/api/resources/__init__.py

lib/pbench/server/api/resources/datasets_metadata.py

lib/pbench/server/api/resources/__init__.py

lib/pbench/test/unit/server/conftest.py

webbnh

Looks very nice. (Adding all the from's is good.) I have a small question and a couple of suggestions.

lib/pbench/server/api/resources/__init__.py

lib/pbench/server/api/resources/server_configuration.py

lib/pbench/test/unit/server/conftest.py

PBENCH-995 The dashboard is sometimes receiving HTTP errors without `{"message"}` JSON payloads. It's not entirely clear why this is happening given the `APIAbort` logic, we could provide more useful hints in most cases and I've tried to "fill in the blanks" in places where we'd omitted the `message` parameter. In the course of this, I also added logic (mimicking that in several other places) to aggregate errors when using `PUT /datasets/metadata` to set multiple keys, when a weird error occurs in just a few. I realized that this error path wasn't *tested*, and added a unit test.

PBENCH-459 Address PBENCH-459 more expansively by A) Limiting internal system error messages to a minimum level of detail while B) Adding a UUID to the client response message and system log, supporting analysis

I'm now reporting a "canned" client response message giving away nothing but the generated UUID, while the internal log message can contain a more detailed description of the error. I moved generation of the log message from the constructor (which was weird), to the _dispatch outer block. The "partial success" cases in `/datasets/delete`, `/datasets/publish`, and in `PUT /datasets/metadata` are now *success* cases with details in the JSON response payload. I'm not 100% happy with this, either, but with the shift to content-free client responses on 500, this seems the new least worst option.

... but not really betting on it.

webbnh

Looks OK to me.

dbutenhof

I have two approvals; I'd appreciate @webbnh resolving conversations if he happens to be on, but I'll likely merge this by Monday anyway just to have it done!

lib/pbench/server/api/resources/server_configuration.py

lib/pbench/server/api/resources/__init__.py

dbutenhof added bug Server API Of and relating to application programming interfaces to services and functions labels Nov 30, 2022

dbutenhof self-assigned this Nov 30, 2022

portante added this to the v0.72 milestone Nov 30, 2022

portante requested changes Nov 30, 2022

View reviewed changes

npalaska reviewed Dec 1, 2022

View reviewed changes

dbutenhof force-pushed the error branch from 470adff to 123a5c2 Compare December 1, 2022 18:07

dbutenhof requested review from npalaska, portante, webbnh and siddardh-ra December 1, 2022 18:08

siddardh-ra previously approved these changes Dec 1, 2022

View reviewed changes

webbnh previously approved these changes Dec 1, 2022

View reviewed changes

dbutenhof dismissed stale reviews from webbnh and siddardh-ra via f7d0287 December 1, 2022 21:47

dbutenhof requested review from webbnh and siddardh-ra December 1, 2022 23:31

webbnh previously approved these changes Dec 2, 2022

View reviewed changes

lib/pbench/test/unit/server/test_datasets_metadata.py Outdated Show resolved Hide resolved

lib/pbench/test/unit/server/test_datasets_metadata.py Show resolved Hide resolved

riya-17 previously approved these changes Dec 2, 2022

View reviewed changes

npalaska previously approved these changes Dec 2, 2022

View reviewed changes

portante mentioned this pull request Dec 2, 2022

Stop using the logger.error("{}", str(exc)) idiom #3099

Open

portante requested changes Dec 2, 2022

View reviewed changes

lib/pbench/server/api/resources/query_apis/datasets/__init__.py Outdated Show resolved Hide resolved

lib/pbench/server/api/resources/datasets_metadata.py Outdated Show resolved Hide resolved

siddardh-ra previously approved these changes Dec 5, 2022

View reviewed changes

dbutenhof marked this pull request as draft December 5, 2022 19:23

dbutenhof dismissed siddardh-ra’s stale review via 2442189 December 5, 2022 21:18

dbutenhof dismissed stale reviews from riya-17 and webbnh via 2442189 December 5, 2022 21:18

dbutenhof force-pushed the error branch from f7d0287 to 2442189 Compare December 5, 2022 21:18

dbutenhof marked this pull request as ready for review December 5, 2022 21:48

dbutenhof requested review from portante, webbnh, npalaska, riya-17 and siddardh-ra December 5, 2022 21:48

webbnh requested changes Dec 6, 2022

View reviewed changes

dbutenhof force-pushed the error branch from e4b373f to 1cf89ce Compare December 9, 2022 16:29

dbutenhof requested a review from webbnh December 9, 2022 16:45

webbnh previously approved these changes Dec 9, 2022

View reviewed changes

dbutenhof added 7 commits December 9, 2022 15:48

Review comments and cleanup

82ac3c0

More revisionisms

b235dba

Expand scope

43c57bd

PBENCH-459 Address PBENCH-459 more expansively by A) Limiting internal system error messages to a minimum level of detail while B) Adding a UUID to the client response message and system log, supporting analysis

Cleanup in aisle 3

7878abf

Hoping this messy merge went smoothly...

a2cbb37

... but not really betting on it.

dbutenhof dismissed webbnh’s stale review via a2cbb37 December 9, 2022 21:38

dbutenhof force-pushed the error branch from 1cf89ce to a2cbb37 Compare December 9, 2022 21:38

webbnh approved these changes Dec 9, 2022

View reviewed changes

portante approved these changes Dec 10, 2022

View reviewed changes

portante mentioned this pull request Dec 10, 2022

Minor cleanups on Auth module #3094

Closed

dbutenhof commented Dec 10, 2022

View reviewed changes

lib/pbench/server/api/resources/server_configuration.py Show resolved Hide resolved

lib/pbench/server/api/resources/__init__.py Show resolved Hide resolved

dbutenhof merged commit b33985f into distributed-system-analysis:main Dec 12, 2022

dbutenhof deleted the error branch December 12, 2022 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API error improvements #3091

API error improvements #3091

dbutenhof commented Nov 30, 2022

portante left a comment

npalaska left a comment

dbutenhof commented Dec 1, 2022

siddardh-ra left a comment

webbnh left a comment

webbnh left a comment

npalaska left a comment

webbnh left a comment •

edited

Loading

webbnh left a comment

webbnh left a comment

dbutenhof left a comment

API error improvements #3091

API error improvements #3091

Conversation

dbutenhof commented Nov 30, 2022

portante left a comment

Choose a reason for hiding this comment

npalaska left a comment

Choose a reason for hiding this comment

dbutenhof commented Dec 1, 2022

siddardh-ra left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

npalaska left a comment

Choose a reason for hiding this comment

webbnh left a comment • edited Loading

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

webbnh left a comment

Choose a reason for hiding this comment

dbutenhof left a comment

Choose a reason for hiding this comment

webbnh left a comment •

edited

Loading