Add support for Correlation ID's across fetch requests and error / ac… #3602
Conversation
# Conflicts: # CHANGELOG.md # admin/tabs/activity/clienterrors/ClientErrorsModel.ts # admin/tabs/activity/tracking/ActivityTrackingModel.ts # admin/tabs/activity/tracking/detail/ActivityDetailModel.ts # admin/tabs/activity/tracking/detail/ActivityDetailView.ts # core/exception/ExceptionHandler.ts # svc/FetchService.ts # svc/TrackService.ts # yarn.lock
# Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
So the main questions I have are around where / how support for automatic ("on by default") correlation IDs should live. Trying to reconcile some competing factors...
- Originating/generating earlier and automatically in
LoadSpecseems powerful / attractive, as this supports a relatively common pattern of making multiple fetches to satisfy a single user request or action.- It also gives you the option of getting a generated ID to pass to some other kind of non-FetchService based request or API that might take one - I don't have a specific example, but seems possible that an app might be using some other library - e.g. a redis or GRPC client - that would be able to consume them.
- With the current impl, we can get a corrId on LoadSpec, but have to call the public load methods with a config object every time - no way to have it generated by default.
- At the same time, I would still like auto ID support to be available on via direct uses of the FetchService API, in case you are calling it directly without having a LoadSpec on hand (common for the many calls apps might make made outside of
doLoadAsync). - Could we select someplace to default-enable and customize generally for both use cases, like the AppSpec or XH?
- I'd also vote to enable by default, so we're talking about a place to disable and potentially override the generation routine.
- FetchService API to customize the header key does of course make sense to live on FetchService.
|
Related to decision on how / if to enable by default - what about for our own admin client? |
| * `FetchOptions.correlationId` - specify an ID to be used on a particular request, if not | ||
| auto-generated by default or already provided via `loadSpec`. | ||
| * `TrackOptions.correlationId` - specify an ID for a tracked activity, if not already | ||
| relayed from a `FetchService` response. |
There was a problem hiding this comment.
We can see where we land with the feature, but this could prob. get cut down a bit.
(In general, realize we need to start building out browsable documentation that's not jammed into the CL, but which we could reference from here. We will get there.)
CHANGELOG.md
Outdated
| @@ -4,9 +4,28 @@ | |||
|
|
|||
| ### 🎁 New Features | |||
|
|
|||
| * Added support for Correlation IDs across fetch requests and error / activity tracking: | |||
| * ⚠️ Upgrade to `hoist-core >= v20.4.0` to enable support for persisting correlation IDs to | |||
There was a problem hiding this comment.
Expecting this can go out in core as the next minor release
All good points here. I see the value in assigning the same Correlation ID to multiple, related requests that share a common Re: enabling by default, I agree that seems reasonable but have encountered client services that throw and return 500's when receiving unexpected headers. Would probably just want to note this as a breaking change if we enabled by default. |
|
Hi -- Sorry for the delay here, and really don't have that much to say, other than to throw out the following "conclusions" that came out of Anselm and my discussion with Tom and John today. Unless you strongly object, we are suggesting:
Anselm please correct me if I missed anything, and happy to continue to hash this out tommorrow. |
Got it. I’d love to talk more this morning, but here are some initial thoughts on having Correlation ID auto-generation live within
|
|
Greg and I had a great talk about this this morning and I think there are
some really good points here. I mentioned to Greg the option of punting on
auto-generation completely in this round, until we get a bit more data.
It's really an additional feature on the
I agree that the id generation specification and code could live on
FetchService, but to be clear we would still need an Alias on XH. If I
want to generate one, I am definitely looking for the terse
`XH.generateCorrelationId()` or perhaps even terser....
…On Wed, Jul 31, 2024 at 8:45 AM Greg Solomon ***@***.***> wrote:
Hi -- Sorry for the delay here, and really don't have *that* much to say,
other than to throw out the following "conclusions" that came out of Anselm
and my discussion with Tom and John today. Unless you strongly object, we
are suggesting:
1. Having the auto installation of correlationIds apply *only* to
LoadSpecs. Its a simpler story to have that behavior in one place, and
LoadSpec is already the higher order object where we are introducing our
"low-code" magic. If this change just makes the entire object more
valuable, great. On the other hand, FetchService is the lower-level API,
and ok to be more explicit there.
2. Accordingly, we want to move the global settings for
generation/autogeneration *off* of FetchService. Specifically, we were
thinking of specifying them as follows:
- XH.correlationIdGenerator (getter/setter)
- XH.defaultCorrelationIdGenerator (getter)
Note We want to lose the concept of a built-in prefix -- you can do
that if you want using your own generator function, and easy access to the
default generator makes falling back to it easy.
- LoadSpec.autoGenerateCorrelationIds (static property)
This could also be placed on XH, but I personally like placing it
closer in the API to the thing it is acting on, and also think it might
benefit conceptually from being seperated from the generator functions.
3. We never want to throw in the case of conflict with specified ids
from multiple sources -- we should just have a simple algorithm to resolve,
and warn in the console at most. We never want our instrumentation code to
bring down the app.
Anselm please correct me if I missed anything, and happy to continue to
hash this out tommorrow.
Got it. I’d love to talk more this morning, but here are some initial
thoughts on having Correlation ID auto-generation live within LoadSpec
and not FetchService:
- I believe many (most?) applications using Correlation IDs would need
to write their own boilerplate to generate Correlation ID’s for calls made
outside of doLoadAsync methods. I imagine there will be a lot of
these, since (at least in my experience) doLoadAsync tends to be used
for fetching core model and service data and not for making POST / PUT
/ DELETE requests, which are arguably some of the most important calls
to trace via Correlation ID in tools like Splunk. I worry that having
automatic Correlation ID generation as a feature of LoadSpec and not
FetchService might confuse unfamiliar developers (Why do I sometimes
need to generate a Correlation ID, and other times it’s generated for me?
Oh, because it’s part of LoadSpec. Should I be creating a LoadSpec
then?)
- I believe it would encourage developers to use the same Correlation
ID for multiple calls within doLoadAsync, which are undoubtedly
related as far as the UI is concerned, but are we sure this is a best
practice? I can imagine a case where 3 calls are made, and one fails. Might
it make it harder to trace the failed call downstream if it shares the same
Correlation ID as other (successful) calls that may be related in the UI
but not in the context of other downstream services? Originally, I had
imagined making Correlation ID a property of LoadSpec would be an
advanced feature and not necessarily a standard practice we’d encourage.
I’m definitely interested in hearing your thoughts on this, however, since
it sounds like you feel differently.
- Understood about FetchService being a lower-level API. For what it’s
worth, given that Correlation ID’s ultimately end up in request headers,
and FetchService already has a method for modifying the default
headers, it didn’t feel out of place to me that default Correlation ID
generation could also live within FetchService, but understood if
others feel differently.
—
Reply to this email directly, view it on GitHub
<#3602 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARTL2JC2AAGU62GF324KR3ZPDL73AVCNFSM6AAAAABEW5HZZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRQGQZTSNJXHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
|
Just pushed new changes here and for Toolbox:
|
# Conflicts: # CHANGELOG.md
…tivity tracking
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.