Fix T1945.
Details
- Reviewers
zack seirl - Maniphest Tasks
- T1945: Return timings instead of simply logging them
- Commits
- rDGRPH7e1917a236f3: server: add endpoints wrapper class to return metadata
Diff Detail
- Repository
- rDGRPH Compressed graph representation
- Branch
- endpoint-return-timings
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 7208 Build 10174: arc lint + arc unit
Event Timeline
I hadn't in mind to lift the timings up to the REST layer, but why not, it will enable doing interesting stuff using non Java clients. However, a couple of change requests:
- timings should not be returned by default in a public facing service, as they might be used as side channels. We should add a server option (either in a configuration file or as a CLI switch) that will enable timings and that is disabled by default. --debug comes to mind, but a more specific --timings would do too. When the option is not given, the returned JSON will not contain the "timings" key at all
- we should wrap the JSON return type more properly. Please use "result" instead of "content", everything else (like "timings" now, possibly other stuff in the future) will be metadata about the result
I did not like the idea of having timings analysis/wrapper class in the low-level Traversal.java, especially since we want to time the node conversions as well so I prefered to move everything in Endpoint.java. ;)
- timings should not be returned by default in a public facing service, as they might be used as side channels. We should add a server option (either in a configuration file or as a CLI switch) that will enable timings and that is disabled by default. --debug comes to mind, but a more specific --timings would do too. When the option is not given, the returned JSON will not contain the "timings" key at all
Ok, I don't know how to do this with Javalin framework but will look into it.
- we should wrap the JSON return type more properly. Please use "result" instead of "content", everything else (like "timings" now, possibly other stuff in the future) will be metadata about the result
True, I will also rename "timings" into "metadata" because I don't like having both "Timing" and "Timings" class name.
- we should wrap the JSON return type more properly. Please use "result" instead of "content", everything else (like "timings" now, possibly other stuff in the future) will be metadata about the result
True, I will also rename "timings" into "metadata" because I don't like having both "Timing" and "Timings" class name.
If you do add a top-level "metadata" key (which I do like, although just "meta" will probably be more idiomatic and consistent with the rest of SWH conventions) you should make "timings", or whatever name you choose for it, a *sub*key of it.
Hence the dictionary should have this structure:
- result
- meta
- timings
- Timings are now optional
- Improve JSON output structure (rename Result class to Output, rename
content member to result, add meta member)
TODO: update documentation (api.rst and server README)