Skip to content
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

allow clients to set QUERY_TAG parameter via context #427

Merged

Conversation

GregOwen
Copy link
Contributor

@GregOwen GregOwen commented May 11, 2021

Description

  1. Add new WithQueryTag method to allow clients to pass the QUERY_TAG parameter to a query via the context
  2. Add test to verify that we're setting the tag correctly

Checklist

  • Code compiles correctly
  • Run make fmt to fix inconsistent formats
  • Run make lint to get lint errors and fix all of them
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

@GregOwen
Copy link
Contributor Author

@sfc-gh-jbahk here's the second of the two PRs I mentioned. This one should hopefully be pretty straightforward.

@sfc-gh-jbahk
Copy link
Contributor

@GregOwen why is this necessary? Could you give me an example of how you intend on using this code?

@GregOwen
Copy link
Contributor Author

GregOwen commented May 12, 2021

@sfc-gh-jbahk Sorry for the delay in getting back to you. Some of our customers have asked us to include Sigma-level metadata (username, Sigma document) for each Snowflake query so that they can do automated attribution of their Sigma-initiated Snowflake usage. Right now, we add this metadata as a JSON blob in a comment at the end of the query, but our customers have asked us to move this information into a separate Query Tag column within Snowflake so that it's easier for them to extract and analyze within Snowflake. We do that by passing the JSON blob as the value for a parameter with the name Query Tag.

@sfc-gh-jbahk
Copy link
Contributor

@GregOwen I see potential conflicts with this approach wherein the statement parameters that the customers might pass into the statement could either throw errors, not accepted by our server or conflict with our internal parameters. Another issue would be security threats including, but not limited to, injections. Could you give me a more specific example? Thanks.

@GregOwen
Copy link
Contributor Author

That makes sense. We probably don't need this to be as wide an interface as it currently is.
Our requirements are:

  1. we can pass a JSON blob containing metadata per-query
  2. the per-query metadata will be associated with the query in a way that Sigma customers can parse and analyze within Snowflake (in particular, we don't want the JSON blob to be part of a comment within the text of the query itself)

The way that we're currently using it is something like this:

params := make(map[string]string)
params["QUERY_TAG"] = fmt.Sprintf(`{"request-id":"%s","email":"%s"}`, requestId, email) // possibly Sigma document ID, etc.
ctx = WithContextStatementParameters(ctx, params)

// execute the query with the given context

@sfc-gh-jbahk
Copy link
Contributor

@GregOwen I see. Where would these parameters be used? Will it be inserted as a value of a column?

@GregOwen
Copy link
Contributor Author

@sfc-gh-jbahk The idea is that the metadata will show up as a Query Tag column in the customer's History view. Here's an example:

Screen Shot 2021-05-13 at 1 50 22 PM

This way a customer's Snowflake admin can use Snowflake to ask questions like "how many queries did Sigma run?" or "which Sigma documents are responsible for my increased Snowflake usage?".

@sfc-gh-jbahk
Copy link
Contributor

@GregOwen I've reviewed this with my team and think it should be okay to move forward but if you can change the code to be less generic such that the statement context doesn't accept anything the user inputs but rather tailor it towards a query tag. Maybe this can be of some use: https://docs.snowflake.com/en/sql-reference/parameters.html#query-tag.

@GregOwen
Copy link
Contributor Author

@sfc-gh-jbahk sounds great - I reworked it to be more precisely targeted at QUERY_TAG and added a test

@GregOwen GregOwen force-pushed the greg/sigma-parameters branch from 51d33e7 to 128fa0b Compare May 19, 2021 01:58
@GregOwen GregOwen changed the title allow clients to set query parameters via context allow clients to set QUERY_TAG parameter via context May 19, 2021
@GregOwen GregOwen force-pushed the greg/sigma-parameters branch from 128fa0b to 12dc39d Compare May 19, 2021 01:59
@sfc-gh-jbahk
Copy link
Contributor

@GregOwen can you rebase and push this branch?

@GregOwen GregOwen force-pushed the greg/sigma-parameters branch from 12dc39d to de1ae3b Compare June 21, 2021 20:24
@GregOwen
Copy link
Contributor Author

@sfc-gh-jbahk done

@GregOwen
Copy link
Contributor Author

Test failures look like they're related to test infrastructure/secrets:

Run ./ci/test.sh
  ./ci/test.sh
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    GOROOT: /opt/hostedtoolcache/go/1.15.13/x64
    PARAMETERS_SECRET: 
    CLOUD_PROVIDER: AWS
gpg: gcry_kdf_derive failed: Invalid data
gpg: decryption failed: No secret key
Error: Process completed with exit code 2.

@GregOwen GregOwen force-pushed the greg/sigma-parameters branch 2 times, most recently from f6a75ea to 9342626 Compare June 25, 2021 22:09
@GregOwen GregOwen force-pushed the greg/sigma-parameters branch from 9342626 to 386fad3 Compare July 19, 2021 16:39
@tampajohn
Copy link

@sfc-gh-jbahk any idea if/when this is planned to be merged? Or what needs to be done to get it mergeable?

@sfc-gh-jbahk
Copy link
Contributor

@sfc-gh-cshi can you take a look at this PR and see if it's something the go driver needs/wants? cc: @sfc-gh-hkapre

@sfc-gh-pfus
Copy link
Collaborator

Hi @tampajohn ! Do you still need this? Can you rebase and solve conflicts and align to new test helper function?

@sfc-gh-pfus
Copy link
Collaborator

Sorry @tampajohn , I meant @GregOwen of course!

@GregOwen
Copy link
Contributor Author

@sfc-gh-pfus yes, we would still like to merge this PR. Can you confirm that somebody at Snowflake will be responsive to changes to this PR? Last time we got it to the 1-yard line but couldn't get it merged because each new commit requires manual re-approval to run the CI tests.

@sfc-gh-pfus
Copy link
Collaborator

Yes, now go driver migrated to another team. I merged/closed/handled 13 PRs this week, so I believe that we can handle one more :)

@GregOwen GregOwen force-pushed the greg/sigma-parameters branch from 386fad3 to 1edd8b3 Compare November 13, 2023 19:59
@GregOwen GregOwen requested a review from a team as a code owner November 13, 2023 19:59
Copy link

github-actions bot commented Nov 13, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@GregOwen
Copy link
Contributor Author

One sec; making sure that Sigma knows about the CLA and is comfortable with me signing it

@debugmiller
Copy link

Would love to see this merged in!

@GregOwen
Copy link
Contributor Author

GregOwen commented Dec 4, 2023

I have read the CLA Document and I hereby sign the CLA

@GregOwen GregOwen force-pushed the greg/sigma-parameters branch from 1edd8b3 to bf55c83 Compare December 4, 2023 16:58
@GregOwen
Copy link
Contributor Author

GregOwen commented Dec 4, 2023

recheck

@jsjqian
Copy link

jsjqian commented Dec 4, 2023

I have read the CLA Document and I hereby sign the CLA

@GregOwen
Copy link
Contributor Author

GregOwen commented Dec 4, 2023

@sfc-gh-pfus I've rebased and we've signed the CLA (it took a while to get legal approval for that). Could you please kick off the tests?

statement_test.go Outdated Show resolved Hide resolved
util.go Show resolved Hide resolved
@sfc-gh-pfus
Copy link
Collaborator

Hi @GregOwen ! Thanks again for your contribution. I left some two suggestions and in the meantime started jobs.

@sfc-gh-pfus
Copy link
Collaborator

Your test hangs. I believe this is because you forgot to defer rows closing.

@GregOwen
Copy link
Contributor Author

GregOwen commented Dec 5, 2023

@sfc-gh-pfus I've pushed changes that should address your comments; let me know if they look good to you

@sfc-gh-pfus
Copy link
Collaborator

sfc-gh-pfus commented Dec 6, 2023

Hi @GregOwen ! All of your checks are green (I fixed a flaky test in the meantime) and the code looks fine. Can you rebase your PR? I can't do it myself.

@GregOwen GregOwen force-pushed the greg/sigma-parameters branch from 8c4885a to 00bb572 Compare December 6, 2023 16:15
@GregOwen
Copy link
Contributor Author

GregOwen commented Dec 6, 2023

@sfc-gh-pfus rebased. It looks like we may get stuck in a "need to rebase" --> "need approval to run tests" loop since we're in different timezones. I'm based in EST; should we try to coordinate tomorrow so that I rebase 9AM my time (which I'm guessing will be towards the end of your workday)?

@sfc-gh-pfus sfc-gh-pfus merged commit cf94c15 into snowflakedb:master Dec 7, 2023
24 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
@sfc-gh-pfus
Copy link
Collaborator

Hi @GregOwen ! Actually I wanted to do it in the morning, before no one from merges from my team ;) Thank you very much, your commit will be released in January!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants