SNOW-1271301 add telemetry fields and use the metrics ExecTimeTelemet… #1762

sfc-gh-dbouassida · 2024-05-21T08:05:05Z

Overview

This PR is still WIP. Feedback is needed for the general logic of the enhanced telemetry.
Tests are to be added.

Pre-review self checklist

PR branch is updated with all the changes from master branch
The code is correctly formatted (run mvn -P check-style validate)
New public API is not unnecessary exposed (run mvn verify and inspect target/japicmp/japicmp.html)
The pull request name is prefixed with SNOW-XXXX:

External contributors - please answer these questions before submitting a pull request. Thanks!

What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

Fixes #NNNN
Fill out the following pre-review checklist:
- I am adding a new automated test(s) to verify correctness of my new code
- I am adding new logging messages
- I am modifying authorization mechanisms
- I am adding new credentials
- I am modifying OCSP code
- I am adding a new dependency or upgrading an existing one
- I am adding new public/protected component not marked with @SnowflakeJdbcInternalApi (note that public/protected methods/fields in classes marked with this annotation are already internal)
Please describe how your code solves the related issue.

Please write a short description of how your code change solves the related issue.

github-actions · 2024-05-21T08:05:21Z

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

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

_{You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.}

sfc-gh-dbouassida · 2024-05-21T09:30:30Z

This PR is still WIP. Feedback is needed for the general logic of the enhanced telemetry.
Tests are to be added.

sfc-gh-dprzybysz

I left some comments - please also think about tests for your changes

src/main/java/net/snowflake/client/core/ExecTimeTelemetryData.java

sfc-gh-dprzybysz · 2024-05-22T08:15:28Z

src/main/java/net/snowflake/client/core/ExecTimeTelemetryData.java

  private String requestId;

  public ExecTimeTelemetryData(String queryFunction, String batchId) {
-    if (TelemetryService.getInstance().isHTAPEnabled()) {
+    if (TelemetryService.getInstance().isHTAPEnabled()
+        || TelemetryService.getInstance().isClientTelemetryEnabled()) {


let's create a single method in TelemetryService to decide if the telemetry is turned on here

sfc-gh-dprzybysz · 2024-05-22T09:51:43Z

src/main/java/net/snowflake/client/core/QueryResultFormat.java

@@ -24,4 +24,9 @@ public static Optional<QueryResultFormat> lookupByName(String n) {

    return Optional.empty();
  }
+
+  @Override
+  public String toString() {


I don't think it should be toString but more getName() or better getParameterValue()

sfc-gh-dprzybysz · 2024-05-22T09:52:24Z

src/main/java/net/snowflake/client/core/SFArrowResultSet.java

@@ -160,6 +160,9 @@ public SFArrowResultSet(
    // update the driver/session with common parameters from GS
    SessionUtil.updateSfDriverParamValues(this.parameters, statement.getSFBaseSession());

+    // send a metric to tell the result format is arrow


i don't think we should keep such comments - the method name is quite expressive

sfc-gh-dprzybysz · 2024-05-22T09:56:49Z

src/main/java/net/snowflake/client/jdbc/telemetry/TelemetryUtil.java

+   * @param value the value to log for the field
+   * @return TelemetryData instance constructed from parameters
+   */
+  public static TelemetryData buildJobData(String queryId, TelemetryField field, String value) {


Can we create a method where the value is Object and make the method above using the new one and make the old method deprecated?

sfc-gh-dprzybysz · 2024-05-22T09:58:32Z

src/main/java/net/snowflake/client/core/ExecTimeTelemetryData.java

@@ -179,6 +190,26 @@ public void addRetryLocation(String location) {

  public String generateTelemetry() {
    if (this.sendData) {
+      if (this.telemetryClient != null) {


it means that we are using this method to generate one event and by the way create 3 other events? I don't think it should happen in one method

also do we want to generate separate events to the batch? cannot we add them to the ExecutionTimeRecord created below?

sfc-gh-dprzybysz · 2024-05-22T12:25:55Z

src/main/java/net/snowflake/client/jdbc/telemetryOOB/TelemetryService.java

@@ -142,6 +164,10 @@ public boolean isHTAPEnabled() {
    }
  }

+  public boolean isTelemetryEnabled() {


should it be isInBoundTelemetryEnabled?

this method is called by the ExecTimeTelemetryData. it is true when isHTAPEnabled or isClientTelemetryEnabled. It can haooen that isClientTelemetryEnabled is false and thus InBoundTelemetry is false, but isHTAPEnabled is true.
The ExecTimeTelemetryData will collect data when either of oob or ib telemetries is enabled.

But it does not seems to be right that on the session parameter we are changing the global state - but it's already merged for a long time

Nevertheless. For sending the new telemetry events you are adding we should be do this via inbound telemetry?

Yes, it should go to to client_telemetry_v with the session and query_id. There are already some fields that are sent, we are trying to add more.

sfc-gh-dprzybysz · 2024-05-22T12:26:16Z

src/main/java/net/snowflake/client/jdbc/telemetryOOB/TelemetryService.java

+    }
+  }
+
+  public boolean isClientTelemetryEnabled() {


could this method be private?

sfc-gh-dprzybysz · 2024-05-22T12:30:50Z

src/main/java/net/snowflake/client/core/SFSession.java

@@ -656,6 +656,11 @@ public synchronized void open() throws SFException, SnowflakeSQLException {
    } else {
      TelemetryService.disableHTAP();
    }
+    if (isClientTelemetryEnabled()) {


inbound telemetry is enabled per session - but here we set it on the static level - do we need it at all? can we just use the flag from the session wherever it's necessary?

SNOW-1271301 entcouple the TelemetryClinet and the TelemetryService SNOW-1271301 entcouple the ExecTimeTelemetryData and the TelemetryClinet SNOW-1271301 add client_time_response_from_server SNOW-1271301 add telemetry fields and use the metrics ExecTimeTelemetryData to send telemetry.

…job data using a value of type Object

sfc-gh-dbouassida force-pushed the SNOW-1271301-jdbc-enhanced-telemetry branch from d317a7f to f58477c Compare May 21, 2024 09:29

sfc-gh-dbouassida requested review from sfc-gh-pbulawa and sfc-gh-pfus May 21, 2024 09:29

sfc-gh-dbouassida force-pushed the SNOW-1271301-jdbc-enhanced-telemetry branch 2 times, most recently from d405797 to ffea8b2 Compare May 21, 2024 09:43

sfc-gh-pfus requested a review from sfc-gh-dprzybysz May 22, 2024 07:37

sfc-gh-dprzybysz reviewed May 22, 2024

View reviewed changes

sfc-gh-dbouassida force-pushed the SNOW-1271301-jdbc-enhanced-telemetry branch from 2453b50 to 9eafabf Compare June 24, 2024 09:02

SNOW-1271301 refactor TelemetryUtil.buildJobData to support building …

17822fa

…job data using a value of type Object

sfc-gh-pbulawa removed their request for review August 27, 2024 13:31

sfc-gh-dbouassida closed this Aug 27, 2024

github-actions bot locked and limited conversation to collaborators Aug 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1271301 add telemetry fields and use the metrics ExecTimeTelemet… #1762

SNOW-1271301 add telemetry fields and use the metrics ExecTimeTelemet… #1762

sfc-gh-dbouassida commented May 21, 2024 •

edited

Loading

github-actions bot commented May 21, 2024

sfc-gh-dbouassida commented May 21, 2024 •

edited

Loading

sfc-gh-dprzybysz left a comment

sfc-gh-dprzybysz May 22, 2024

sfc-gh-dprzybysz May 22, 2024

sfc-gh-dprzybysz May 22, 2024

sfc-gh-dprzybysz May 22, 2024

sfc-gh-dprzybysz May 22, 2024

sfc-gh-dprzybysz May 22, 2024

sfc-gh-dbouassida May 22, 2024

sfc-gh-dprzybysz May 22, 2024

sfc-gh-dbouassida May 22, 2024

sfc-gh-dprzybysz May 22, 2024

sfc-gh-dprzybysz May 22, 2024

SNOW-1271301 add telemetry fields and use the metrics ExecTimeTelemet… #1762

SNOW-1271301 add telemetry fields and use the metrics ExecTimeTelemet… #1762

Conversation

sfc-gh-dbouassida commented May 21, 2024 • edited Loading

Overview

Pre-review self checklist

External contributors - please answer these questions before submitting a pull request. Thanks!

github-actions bot commented May 21, 2024

sfc-gh-dbouassida commented May 21, 2024 • edited Loading

sfc-gh-dprzybysz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-dbouassida commented May 21, 2024 •

edited

Loading

sfc-gh-dbouassida commented May 21, 2024 •

edited

Loading