Support `correlationId` across fetch/HTTP calls and error/activity tracking

[lbwexler]

We would like to add support for generating/supporting CorrelationIds for applications. Some elements we would like to see in any solution:

• Easy ability to generate unique id for Fetch call kicking off the operation.
• Built-in integration to usage tracking/client error tracking/logging
• Support in HttpClient/JsonClient to pass through ids in requests/responses

Will require both hoist-react and hoist-core releases

[amcclain]

Here are some ideas / specs I wrote up to get ready for us to work on this - happy to discuss and refine further:

FetchService

• Support new FetchOptions.correlationId param - takes a string ID that it will pass along unmodified, or true to install a generated UUID.
  • Question - should we generate and store the ID within LoadSpec? That might make it easier to coordinate several requests that should share a correlation ID - we often have a pattern where e.g. a top-level doLoadAsync call fires off several requests, and we pass the same loadSpec to all of them.
    • This could potentially work in concert with a FetchOptions flag - i.e. if you pass {loadSpec, correlationId: true} the service will use the generated ID on the loadSpec, but could still support {correlationId: true} without a loadSpec, where the service would generate the ID internally.
    • This would also provide a way for the app to access the correlationId generated for / used in a load call for e.g. a downstream exception that is thrown after the call itself has completed.
    • Would also support track() usage (see below).
    • Need to also consider a case where an API is handing back correlationIds even if we did not request them - thinking we would should always check the response for a correlationId and prioritize that / install it on the loadSpec. For any well-behaved API it should be the same one we sent, but again we might also get one back without sending one in the first place - don't think we should drop it.
    • While looking at this - FetchOptions.loadSpec currently typed as PlainObject - shouldn't we use the class here? Do we actually support passing a plain object here?
  • Provide some way to enable generated correlationIds on all requests by default.
  • Provide a configurable way to set the header to be used for setting/reading correlation IDs. We would go with x-correlation-id by default, but we might have a system that uses a variation like x-request-id. Could support setting to a token like none to disable sending/reading corrIds entirely.

ExceptionHandler & ClientError

• Add HoistException.correlationId and ensure exceptions thrown from fetch calls relay any correlationId from the response (including cases where Hoist might not spec an ID on the outbound request, but an API endpoint responds with a corrId on its own).
• Add ExceptionHandlerOptions.correlationId and ExceptionHandlerLoggingOptions.correlationId, or read directly from exception passed to handleException(). Include as param to xh/submitError.
• Add ClientError.correlationId to persist within hoist-core, display within Admin console.

TrackService & TrackLog

• Support new TrackOptions.correlationId param, or read from provided loadSpec. Include as param to xh/track.
• Add TrackLog.correlationId to persist within hoist-core, display within Admin console.

JSONClient (maybe?)

• Support new correlationId argument to public executeAsXXX() APIs? Either as a top-level String arg or in a new options Map arg (to allow for future features).
• Install for outbound request in configured header.
• Read from response within parseException, try to get into HoistException if possible???

[lbwexler]

• Very much in favor of leveraging LoadSpec
• Agree with everything else in here.
• Am curious about what we mean by "always generate" a correlation id by default. I don't think we would want these ids on our Hoist API fetches -- sending the header would be fine, but the various logging might just be noise. Should we start with the requirement that you need to be explicit?
• do I dare suggest a shorter name -- 'corrId', cKey, corrKey, cid, 'ck', processKey?

[amcclain]

• Thanks for mentioning the logging! That's one thing I didn't capture here. Believe we want to have some out-of-the-box handling within our Hoist logging to log correlationId if present, similar to what we do with username.

  • But then yes, agree we will want some control over that to avoid it being too noisy - i.e. it will be essential to relay in machine-readable logs, but less so for humans reading logs in the admin console log viewer...

• Wondering if we should have a convenience method in our BaseController/BaseService class to extract the correlationId from the configured header?

• I do believe we want some way to have an ID always generated for requests, otherwise an app developer is going to need to remember to add this on each call to a business API, and will inevitably forget a few / do so inconsistently. Feel pretty strongly that the only way to get this into actual, widespread use is to make it easy to turn on globally in a "set and forget" fashion.

  • But agree 100% that forces careful choices around logging defaults - we could also consider some special handling to change the default behavior when calling our own Hoist endpoints, where having a correlationId is probably not useful.

• Also feel strongly we need to stick with correlationId - it's the term of art and a strong convention in many, many other toolkits.

  • Could consider shortening to corrId but not really a fan of those super-compressed arg names - with TS we will get auto-complete / support for catching typos, and then it's just that much more clear what's happening.
  • With support for defaults, I would expect many apps will have few to no references to this arg anyway.

[lbwexler]

• Its fine to stick with correlationId -- not quite so convinced at the strength of the convention, but it seems safest. On balance, if we are going to be seeing this everywhere, I would choose the shorter corrId version. Would prefer not to have both, which we sometimes

• If its in the header, I guess it won't be disturbing me when troublshooting the network traffic, so that's for sure a plus.

• Not sure there is going to be an elegant way to turn it off in the logging for human readable logs -- think the idea that they both have the same data is a fairly important invariant, and we will often want it in our human readable logs. I think we will probably need to be able to just turn off its generation at the cliientSource with corrId: false

• Random-thought -- Maybe we could log just a small portion of it in our human-readable logs, and log the full id for every request to a seperate log?

[amcclain]

Maybe we could log just a small portion of it in our human-readable logs

I think this is a great idea - could take the first seven chars, ala short hashes on git commits. That's almost certainly going to be unique within any given troubleshooting exercise / analysis.

[lbwexler]

yea, but was a little worried about the constraint that puts on what kind of correlationId you can use.

[amcclain]

yea, but was a little worried about the constraint that puts on what kind of correlationId you can use.

Yes - I was looking for any mention of selecting some more meaningful corrId - e.g. one that started with an app name or included other human-readable info, presumably with some UUID tacked on at the end to ensure uniqueness. Did not find any such examples.

Seemed like it could helpful, but in the end decided that this did not need to be an area where we got more clever than was required, especially if it makes it more difficult for us to take other optimizations like in the logging that we know we would like to take...

[amcclain]

If you think about it - anything you bake into a corrId might just end up being more confusing than it is helpful as the ID gets passed around through a distributed system. E.g. I was thinking originatingApp=Toolbox&[randomGUIDtail] - seems useful to tell you that Toolbox kicked off the entire chain. But then imagine that you have TB > API-A > API-B, and API-A is misbehaving and calling API-B in a bad way, but passing along our ID.

[ghsolomon]

HoistReact PR: https://github.com/xh/hoist-react/pull/3602 HoistCore PR: https://github.com/xh/hoist-core/pull/338 Toolbox PR: https://github.com/xh/toolbox/pull/704