-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Ensure that createViewSession preserves the trace token #25716
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
base: master
Are you sure you want to change the base?
Ensure that createViewSession preserves the trace token #25716
Conversation
@@ -661,6 +661,7 @@ public Session createViewSession(Optional<String> catalog, Optional<String> sche | |||
.setRemoteUserAddress(getRemoteUserAddress().orElse(null)) | |||
.setUserAgent(getUserAgent().orElse(null)) | |||
.setClientInfo(getClientInfo().orElse(null)) | |||
.setTraceToken(traceToken) |
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.
nit: .setTraceToken(getTraceToken())
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.
Thanks, I should've caught that.
ab8ebfe
to
323b87a
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.
Looks simple enough. Is it possible to add a test?
This looks good to me, but if a test is possible, please add that. If not (or really annoying), please let us know, and one of us will merge it as is. |
@huw0 For testing I think we could use |
f82bae1
to
5e91edf
Compare
I've added some unit testing around |
Description
Session
.createViewSession
creates a new Session that is used when analysing views.Presently this does not pass through the trace token. This is breaking the use of views with our custom connectors which mandate that this token is populated.
Therefore this is a very simple PR to ensure that the trace token is populated in
createViewSession
if it is present on the outer session.Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: