-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce transaction_id policy #1460
Introduce transaction_id policy #1460
Conversation
dd81bf7
to
8c47c71
Compare
8c47c71
to
9c2eb58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codewise LGTM
Just some nitpicking done
Regarding logger, it has nothing to do with the PR. I guess the logging will be more relevant for the coming FAPI policy.
Also related to logging. If relying only in the existing logging policy for logging x-fapi-interaction-id
header value, then the customer needs to have custom logging. The customer loses the "standard" logging format (unless it formats in the same way).
[09/May/2024:12:37:29 +0000] one:8080 172.21.0.1:58962 "GET /test?user_key= HTTP/1.1" 200 646 (0.390) 0
Have you considered adding yet another log line (INFO level) with the transaction ID? Maybe not a good idea.
if not transaction_id or transaction_id == "" then | ||
transaction_id = context.transaction_id | ||
end | ||
ngx.header[self.header_name] = transaction_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I wonder if this ngx.header[self.header_name] = transaction_id
statement should only be executed in case it is not in the headers or empty.
The difference in behavior regarding what you coded is when the response header exists but it is empty. in your case, request transaction ID has preference over empty response header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference in behavior regarding what you coded is when the response header exists but it is empty. in your case, request transaction ID has preference over empty response header.
wouldn't transaction_id == ""
already covered that?
Regarding the logging, we have customer complained about extra log line, see THREESCALE-5225. I think the best solution is to inject this ID the the log, similar with what we did for the request-id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right. I got confused.
The only diff would be that you are setting the value even when it is already set. Which is harmless. It is a matter of style. LGTM.
I recommend rebasing this and test this with the new openresty 1.21.4.3 |
Close in favor of #1465 |
What
This PR partially support https://issues.redhat.com/browse/THREESCALE-10973. Specially it satisfy the following 2 requirements from the FAPI - baseline profile
Why making a new policy and not extend the current Header Modification policy
We can easily extend the current Header Policy and add
uuid
filter and new option to generate the header in response if it does not exist in request. However, in my opinion this would complicate the logic of the Title Modification policy. For example, If the user sets theinclude_in_response
checkbox while also setting a header in the Response field, and upstream also returns the same header, which header takes precedence?Also the benefit of having separate policy so we can later extend the support algorithm if need (currently only uuidv4 is supported)
Answer to questions from https://issues.redhat.com/browse/THREESCALE-6577
The policy won't modify the header if it exist
Yes, user should be able to log the header with Logging policy
We use UUID to the change of collision is fairly small
I don't have answer to this but I think it's still useful to have this policy as not everyone use OpenTracing.
Verification steps:
APICAST_IP=$(docker inspect apicast_build_0-development-1 | yq e -P '.[0].NetworkSettings.Networks.apicast_build_0_default.IPAddress' -)
Check the log for the following line
The response header should also contain the
X-Transaction-ID
header