Label BigQuery jobs with Trino query id #16187

wendigo · 2023-02-20T15:56:14Z

Description

It makes it easier to track the origin of BQ DDL and DML queries.

Release notes

(x) This is not user-visible or docs only and no release notes are required.

kokosing

Second commit lgtm

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java

wendigo · 2023-02-21T09:55:10Z

@ebyhr can you run BQ tests?

ebyhr · 2023-02-21T10:03:20Z

/test-with-secrets sha=915be3f3e808f27aa0c2b684f342147a5a9a7173

github-actions · 2023-02-21T18:31:21Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4233150724

ebyhr

The commit order looks a little weird. For instance, 1st commit adds ConnectorSession to BigQueryClient.query, and 2nd commit removes it. I'm fine with squashing commits into one.

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryQueryPageSource.java

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryPageSourceProvider.java

wendigo · 2023-03-28T09:28:43Z

@ebyhr PTAL

wendigo · 2023-05-04T14:52:31Z

@ebyhr @hashhar ptal and run with secrets

hashhar · 2023-05-08T06:24:29Z

/test-with-secrets sha=7c1ee905b13cecf03ab56cf27b5405f0b53f4a21

github-actions · 2023-05-08T07:35:33Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4912226543

wendigo · 2023-05-08T09:27:29Z

@ebyhr ptal. I needed to revert to the previous approach. Please see a rationale in the commit message why passing ConnectorSession is actually needed.

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClientFactory.java

plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java

kokosing

Is it possible to have a test for it? To ask BQ what labels were set when Trino did a query?

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java

wendigo · 2023-05-08T11:10:24Z

@kokosing it's not possible to have this test with a current set of privileges that the credential key has.

hashhar · 2023-05-08T12:59:46Z

Also I don't think it really needs elevated permissions, I see and can query from information schema https://cloud.google.com/bigquery/docs/viewing-labels

The another commit was pushed since the last review

wendigo · 2023-05-09T09:20:09Z

@hashhar @kokosing test added. I hope that permissions will be working here.

hashhar

LGTM % format-interpolation feature

hashhar · 2023-05-09T12:31:07Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConfig.java

+
+    @Config("bigquery.job.label-format")
+    @ConfigDescription("Adds `trino_query` label to the BigQuery job with provided value format")
+    public BigQueryConfig setQueryLabelFormat(String queryLabelFormat)


while it's useful to use common format across connectors for BigQuery it's probably less powerful in the way we use it since all information gets stuffed into single label in BigQuery - I feel this is prematurely adding a feature which would make it harder to evolve over time as people request changes.

What do you think @kokosing @ebyhr - should we not add it at the moment and just label the query id for now?

Alternatively we can convert the config into a boolean toggle and if enabled add one label per each pre-defined placeholder.

I think we should allow to define the label key. So people may organize it the way the like.

I am under impression that the current setup make is very customizable so one can use just query id for labelling. The other can format the label as JSON and parse it later.

airlift/airlift#1066

hashhar · 2023-05-09T12:31:40Z

plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java

+    @Test
+    public void testQueryLabel()
+    {
+        String materializedView = "test_query_label" + randomNameSuffix();


is there a specific reason to use an MV? Add a comment or simplify.

Yes, labels are added to jobs and jobs are used only when querying non-storage-backed relations (i.e. views)

hashhar · 2023-05-09T12:32:42Z

/test-with-secrets sha=00b04af367fe3febd59ae472a4f9fa5447f87652

plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/logging/FormatInterpolator.java

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java

kokosing · 2023-05-09T12:57:23Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConfig.java

+
+    @Config("bigquery.job.label-format")
+    @ConfigDescription("Adds `trino_query` label to the BigQuery job with provided value format")
+    public BigQueryConfig setQueryLabelFormat(String queryLabelFormat)


I think we should allow to define the label key. So people may organize it the way the like.

I am under impression that the current setup make is very customizable so one can use just query id for labelling. The other can format the label as JSON and parse it later.

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConfig.java

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryConnectorModule.java

plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java

wendigo · 2023-05-09T14:19:57Z

@kokosing @hashhar airlift/airlift#1066 this would allow this feature to be generic i.e. defining labels as:

bigquery.job.labels.label1 = $QUERY_ID
bigquery.job.labels.another-label = $TRACE_TOKEN

Asking the user to input JSON and parsing it is adding a complexity here, not hiding it from a user.

github-actions · 2023-05-09T19:19:47Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4926624071

wendigo · 2023-05-15T13:45:57Z

@hashhar @kokosing there won't be a possibility to add much information to this label value as it's limited to 63 chars, and lowercase letters, digits, underscore and hyphen. No JSON possible

ConnectorSession needs to be passed to query/update methods because BigQueryClient is cached using identityCacheMapping.getRemoteUserCacheKey() which is not taking into account session properties. We need also to access queryId in order to properly label queries but we don't want to cache client per query id.

wendigo · 2023-05-16T04:16:32Z

PTAL @hashhar @kokosing

hashhar

LGTM.

Eventually we should plan to have multiple labels instead of adding all information in a single label.

hashhar · 2023-05-16T05:13:45Z

/test-with-secrets sha=56af27891d3001a0358bfd5b1bb4e3f76dee4c6c

github-actions · 2023-05-16T07:40:21Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4988294291

hashhar · 2023-05-16T08:33:32Z

Do we want to not document this/mention in release notes until we have ability to add separate labels?

@kokosing @wendigo @ebyhr ?

kokosing · 2023-05-16T08:38:55Z

I would prefer to document this as it is today.

cla-bot bot added the cla-signed label Feb 20, 2023

wendigo requested review from ebyhr, kokosing and hashhar February 20, 2023 15:56

kokosing reviewed Feb 20, 2023

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java Outdated Show resolved Hide resolved

electrum reviewed Feb 20, 2023

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java Outdated Show resolved Hide resolved

ebyhr reviewed Feb 20, 2023

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java Outdated Show resolved Hide resolved

wendigo requested a review from ebyhr February 21, 2023 09:54

wendigo force-pushed the serafin/label-bq-queries branch from 06045c6 to 915be3f Compare February 21, 2023 09:54

wendigo requested a review from kokosing February 21, 2023 09:55

ebyhr approved these changes Feb 22, 2023

View reviewed changes

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

ebyhr force-pushed the serafin/label-bq-queries branch from 915be3f to ebe9ec2 Compare March 22, 2023 04:33

ebyhr mentioned this pull request Mar 22, 2023

Test: Label BigQuery jobs with Trino query id #16665

Closed

github-actions bot added the bigquery BigQuery connector label Mar 22, 2023

wendigo force-pushed the serafin/label-bq-queries branch from ebe9ec2 to 9055938 Compare March 28, 2023 09:28

wendigo requested a review from ebyhr March 28, 2023 09:28

wendigo force-pushed the serafin/label-bq-queries branch from 9055938 to 7c1ee90 Compare May 4, 2023 14:52

wendigo force-pushed the serafin/label-bq-queries branch from 7c1ee90 to 32618b7 Compare May 8, 2023 09:26

ebyhr previously approved these changes May 8, 2023

View reviewed changes

kokosing reviewed May 8, 2023

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java Outdated Show resolved Hide resolved

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java Outdated Show resolved Hide resolved

wendigo force-pushed the serafin/label-bq-queries branch 2 times, most recently from 81ff158 to 1fb60a5 Compare May 8, 2023 14:10

wendigo force-pushed the serafin/label-bq-queries branch from 1fb60a5 to 00b04af Compare May 9, 2023 09:18

hashhar reviewed May 9, 2023

View reviewed changes

kokosing reviewed May 9, 2023

View reviewed changes

wendigo force-pushed the serafin/label-bq-queries branch from 00b04af to a68ab2e Compare May 15, 2023 13:44

wendigo requested review from hashhar, kokosing and ebyhr May 15, 2023 13:45

wendigo added 2 commits May 15, 2023 21:16

Extract FormatInterpolator to plugin-toolkit

f122fbe

wendigo force-pushed the serafin/label-bq-queries branch from a68ab2e to 56af278 Compare May 15, 2023 19:20

hashhar approved these changes May 16, 2023

View reviewed changes

hashhar merged commit c999795 into trinodb:master May 16, 2023

hashhar mentioned this pull request May 16, 2023

Release notes for 418 #17450

Closed

wendigo deleted the serafin/label-bq-queries branch May 16, 2023 10:08

github-actions bot added this to the 418 milestone May 16, 2023

colebow mentioned this pull request May 17, 2023

Add Trino 418 release notes #17540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Label BigQuery jobs with Trino query id #16187

Label BigQuery jobs with Trino query id #16187

wendigo commented Feb 20, 2023 •

edited by ebyhr

Loading

kokosing left a comment

wendigo commented Feb 21, 2023

ebyhr commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

ebyhr left a comment

wendigo commented Mar 28, 2023

wendigo commented May 4, 2023 •

edited

Loading

hashhar commented May 8, 2023

github-actions bot commented May 8, 2023

wendigo commented May 8, 2023

kokosing left a comment

wendigo commented May 8, 2023

hashhar commented May 8, 2023

wendigo commented May 9, 2023

hashhar left a comment

hashhar May 9, 2023

kokosing May 9, 2023

wendigo May 9, 2023

hashhar May 9, 2023

wendigo May 9, 2023

hashhar commented May 9, 2023

kokosing May 9, 2023

wendigo commented May 9, 2023

github-actions bot commented May 9, 2023

wendigo commented May 15, 2023

wendigo commented May 16, 2023

hashhar left a comment

hashhar commented May 16, 2023

github-actions bot commented May 16, 2023

hashhar commented May 16, 2023

kokosing commented May 16, 2023

Label BigQuery jobs with Trino query id #16187

Label BigQuery jobs with Trino query id #16187

Conversation

wendigo commented Feb 20, 2023 • edited by ebyhr Loading

Description

Release notes

kokosing left a comment

Choose a reason for hiding this comment

wendigo commented Feb 21, 2023

ebyhr commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

ebyhr left a comment

Choose a reason for hiding this comment

wendigo commented Mar 28, 2023

wendigo commented May 4, 2023 • edited Loading

hashhar commented May 8, 2023

github-actions bot commented May 8, 2023

wendigo commented May 8, 2023

kokosing left a comment

Choose a reason for hiding this comment

wendigo commented May 8, 2023

hashhar commented May 8, 2023

wendigo commented May 9, 2023

hashhar left a comment

Choose a reason for hiding this comment

hashhar May 9, 2023

Choose a reason for hiding this comment

kokosing May 9, 2023

Choose a reason for hiding this comment

wendigo May 9, 2023

Choose a reason for hiding this comment

hashhar May 9, 2023

Choose a reason for hiding this comment

wendigo May 9, 2023

Choose a reason for hiding this comment

hashhar commented May 9, 2023

kokosing May 9, 2023

Choose a reason for hiding this comment

wendigo commented May 9, 2023

github-actions bot commented May 9, 2023

wendigo commented May 15, 2023

wendigo commented May 16, 2023

hashhar left a comment

Choose a reason for hiding this comment

hashhar commented May 16, 2023

github-actions bot commented May 16, 2023

hashhar commented May 16, 2023

kokosing commented May 16, 2023

wendigo commented Feb 20, 2023 •

edited by ebyhr

Loading

wendigo commented May 4, 2023 •

edited

Loading