gosnowflake
gosnowflake copied to clipboard
allow clients to set QUERY_TAG parameter via context
Description
- Add new
WithQueryTag
method to allow clients to pass theQUERY_TAG
parameter to a query via the context - Add test to verify that we're setting the tag correctly
Checklist
- [X] Code compiles correctly
- [ ] Run
make fmt
to fix inconsistent formats - [X] Run
make lint
to get lint errors and fix all of them - [X] Created tests which fail without the change (if possible)
- [x] All tests passing
- [ ] Extended the README / documentation, if necessary
@sfc-gh-jbahk here's the second of the two PRs I mentioned. This one should hopefully be pretty straightforward.
@GregOwen why is this necessary? Could you give me an example of how you intend on using this code?
@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
.
@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.
That makes sense. We probably don't need this to be as wide an interface as it currently is. Our requirements are:
- we can pass a JSON blob containing metadata per-query
- 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
@GregOwen I see. Where would these parameters be used? Will it be inserted as a value of a column?
@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:

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?".
@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.
@sfc-gh-jbahk sounds great - I reworked it to be more precisely targeted at QUERY_TAG
and added a test
@GregOwen can you rebase and push this branch?
@sfc-gh-jbahk done
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.
@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-cshi can you take a look at this PR and see if it's something the go driver needs/wants? cc: @sfc-gh-hkapre
Hi @tampajohn ! Do you still need this? Can you rebase and solve conflicts and align to new test helper function?
Sorry @tampajohn , I meant @GregOwen of course!
@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.
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 :)
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅
One sec; making sure that Sigma knows about the CLA and is comfortable with me signing it
Would love to see this merged in!
I have read the CLA Document and I hereby sign the CLA
recheck
I have read the CLA Document and I hereby sign the CLA
@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?
Hi @GregOwen ! Thanks again for your contribution. I left some two suggestions and in the meantime started jobs.
Your test hangs. I believe this is because you forgot to defer rows closing.
@sfc-gh-pfus I've pushed changes that should address your comments; let me know if they look good to you
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.
@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)?