Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

huw0
Copy link
Member

@huw0 huw0 commented May 1, 2025

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:

## General
* createViewSession should preserve the client trace token ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 1, 2025
@huw0 huw0 marked this pull request as ready for review May 1, 2025 20:00
@Praveen2112 Praveen2112 requested review from dain, ksobolew and kokosing May 2, 2025 13:56
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: .setTraceToken(getTraceToken())

Copy link
Member Author

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.

@huw0 huw0 force-pushed the preserve-trace-token-in-createviewsession branch from ab8ebfe to 323b87a Compare May 2, 2025 17:42
Copy link
Contributor

@ksobolew ksobolew left a 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?

@dain
Copy link
Member

dain commented May 5, 2025

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.

@Praveen2112
Copy link
Member

@huw0 For testing I think we could use MockConnectorMetadata which could make use of the trace token at the time of its execution.

@huw0 huw0 force-pushed the preserve-trace-token-in-createviewsession branch from f82bae1 to 5e91edf Compare May 12, 2025 20:44
@huw0
Copy link
Member Author

huw0 commented May 12, 2025

I've added some unit testing around createViewSession and rebased the branch.
There seem to be some test failures caused by github rate limiting and some random plugin failures that change every time. Expect these should pass once any wider issues resolve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants