Page MenuHomeSoftware Heritage

Endpoints now return timings instead of logging them
ClosedPublic

Authored by haltode on Aug 8 2019, 3:53 PM.

Diff Detail

Repository
rDGRPH Graph service
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

haltode created this revision.Aug 8 2019, 3:53 PM
zack requested changes to this revision.Aug 8 2019, 4:26 PM

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
This revision now requires changes to proceed.Aug 8 2019, 4:26 PM
haltode added a comment.EditedAug 8 2019, 4:39 PM
In D1832#42599, @zack wrote:

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.

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.

zack added a comment.Aug 8 2019, 4:43 PM
  • 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
haltode updated this revision to Diff 6176.Aug 8 2019, 11:54 PM
  • 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)

haltode updated this revision to Diff 6177.Aug 9 2019, 9:48 AM

Update documentation (api.rst and server README).

haltode updated this revision to Diff 6178.Aug 9 2019, 9:51 AM

Update JSON output format in api.rst (content -> result, add meta dict).

zack accepted this revision.Aug 9 2019, 10:03 AM
This revision is now accepted and ready to land.Aug 9 2019, 10:03 AM
This revision was automatically updated to reflect the committed changes.