[SHIP-832] Request ID python plumbing#529
Conversation
eob
left a comment
eob
left a comment
There was a problem hiding this comment.
LGTM!
One small idea for a powerup, and it looks like a test is failing, but the design & testing strategy are awesome
| ) # invoke_later tasks get stored b64 encoded | ||
| return InvocableResponse( | ||
| json={ | ||
| "packageRequestId": self.client.config.request_id, |
There was a problem hiding this comment.
This is a clever testing strategy
| _, | ||
| ): | ||
| result = instance.invoke("requestids", plugin_handle=plugin.handle) | ||
| package_request_id = result.get("packageRequestId") |
There was a problem hiding this comment.
Not necessary in this PR, but an awesome addition would be to have the Proxy stamp a header on all requests that contained the requestId.
Then in addition to assert package_requestId is not None we could check against the Proxy's opinion of events.
I guess that would require us to use a raw HTTP post instead of instance.invoke to test though
There was a problem hiding this comment.
This is a good idea; should be very easy to add to the engine PR
GitOnUp
left a comment
GitOnUp
left a comment
There was a problem hiding this comment.
One point of discussion here but LGTM
| profile: Optional[str] = None | ||
|
|
||
| # For use in deployed packages and plugins for tracing. Do not set manually | ||
| request_id: Optional[str] = None |
There was a problem hiding this comment.
Not blocking: Do not set manually is a surprising thing to read without being as ingrained in the context here yet; it also feels at first blush to me like this is Request-Scoped rather than Client-Scoped.
e.g. in the RequestIdGenerator in tests the PluginRequest seems like a natural place this would be, but I'm blanking on a request-scoped place to do this in e.g. a PackageService.
There was a problem hiding this comment.
Yeah, you're right of course...
The reason I tucked it into the client config object specifically and not the InvocationContext was that the client object would need it to send back with every single request to the Engine. So putting it there allowed me to do it without the package or plugin author ever having to know it existed.
The comment, then, is to say "hey, don't mess with this" since AFAIK there isn't a particularly good Pythonic way of actively preventing someone from changing 'internal' member vars.
There was a problem hiding this comment.
_ is usually the shorthand for "don't mess with this", but if you don't mind if they're making decisions by reading it, you could expose a getter without the _
Good idea! Test won't pass until the engine PR is deployed :) |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
4 participants
Receive a requestId in lambda handler and attach it to the client object.
This requestId is then passed in all engine api calls to create traceability throughout the system.
Relies on PR: https://github.com/nludb/nludb/pull/621, so will not pass tests until that is deployed.