gosnowflake icon indicating copy to clipboard operation
gosnowflake copied to clipboard

allow clients to set QUERY_TAG parameter via context

Open GregOwen opened this issue 3 years ago • 14 comments

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

  • [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

GregOwen avatar May 11 '21 22:05 GregOwen

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

GregOwen avatar May 11 '21 22:05 GregOwen

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

sfc-gh-jbahk avatar May 11 '21 22:05 sfc-gh-jbahk

@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 avatar May 12 '21 23:05 GregOwen

@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.

sfc-gh-jbahk avatar May 13 '21 18:05 sfc-gh-jbahk

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

GregOwen avatar May 13 '21 20:05 GregOwen

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

sfc-gh-jbahk avatar May 13 '21 20:05 sfc-gh-jbahk

@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?".

GregOwen avatar May 13 '21 21:05 GregOwen

@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 avatar May 17 '21 23:05 sfc-gh-jbahk

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

GregOwen avatar May 19 '21 01:05 GregOwen

@GregOwen can you rebase and push this branch?

sfc-gh-jbahk avatar Jun 21 '21 16:06 sfc-gh-jbahk

@sfc-gh-jbahk done

GregOwen avatar Jun 21 '21 20:06 GregOwen

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 avatar Jun 21 '21 23:06 GregOwen

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

tampajohn avatar May 20 '22 16:05 tampajohn

@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-jbahk avatar May 20 '22 22:05 sfc-gh-jbahk

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

sfc-gh-pfus avatar Oct 18 '23 09:10 sfc-gh-pfus

Sorry @tampajohn , I meant @GregOwen of course!

sfc-gh-pfus avatar Oct 18 '23 09:10 sfc-gh-pfus

@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.

GregOwen avatar Oct 19 '23 19:10 GregOwen

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 :)

sfc-gh-pfus avatar Oct 20 '23 07:10 sfc-gh-pfus

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

github-actions[bot] avatar Nov 13 '23 19:11 github-actions[bot]

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

GregOwen avatar Nov 13 '23 20:11 GregOwen

Would love to see this merged in!

debugmiller avatar Nov 30 '23 17:11 debugmiller

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

GregOwen avatar Dec 04 '23 16:12 GregOwen

recheck

GregOwen avatar Dec 04 '23 18:12 GregOwen

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

jsjqian avatar Dec 04 '23 19:12 jsjqian

@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?

GregOwen avatar Dec 04 '23 19:12 GregOwen

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

sfc-gh-pfus avatar Dec 05 '23 10:12 sfc-gh-pfus

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

sfc-gh-pfus avatar Dec 05 '23 11:12 sfc-gh-pfus

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

GregOwen avatar Dec 05 '23 16:12 GregOwen

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 avatar Dec 06 '23 10:12 sfc-gh-pfus

@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)?

GregOwen avatar Dec 06 '23 16:12 GregOwen