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 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

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
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.

  • 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)

Update documentation (api.rst and server README).

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

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.