-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: add instrumentation for a couple of OpenTelemetry metrics #2501
feat: add instrumentation for a couple of OpenTelemetry metrics #2501
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
5ad2501
to
c99bd10
Compare
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Show resolved
Hide resolved
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Outdated
Show resolved
Hide resolved
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Outdated
Show resolved
Hide resolved
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Outdated
Show resolved
Hide resolved
76dba99
to
b504448
Compare
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Outdated
Show resolved
Hide resolved
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Outdated
Show resolved
Hide resolved
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Show resolved
Hide resolved
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Show resolved
Hide resolved
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Outdated
Show resolved
Hide resolved
b504448
to
ff5529a
Compare
ff5529a
to
b526bb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM only one comment.
...oud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/ConnectionWorker.java
Outdated
Show resolved
Hide resolved
if (this.traceId.toLowerCase().indexOf("dataflow:") >= 0) { | ||
String[] traceIdParts = this.traceId.split(":", 8); | ||
for (int i = 0; i < traceIdParts.length; i++) { | ||
if (traceIdParts[i].toLowerCase().indexOf("dataflow") >= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need a loop here? Can we remove L326 and just start with traceIdParts[0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are cases where the "dataflow" value can appear in the first or the second part. Here are some examples of traceId values taken from actual logs:
Dataflow:2024-04-30_08_42_19-12633720189001946338
java-streamwriter:HEAD+20240508-1544 Dataflow:monorail-h-multi:2024-05-13_19_05_36-9076476533873711727:5306357987650008794
java-streamwriter Dataflow:pubsub-to-bq-staging-tongruil-1024-static:2024-05-14_15_13_14-5530509399715326669:7308531029326585036
java-streamwriter:HEAD+20240508-1544 Dataflow:monorail-c-multi:2024-05-08_11_44_34-6968230696879535523:2379627307761333914
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O, I think I know why, the HEAD+20240508-1544 is supposed to be the version number but obviously it is a wrong value (could also be the head you look at are from head client libs).... How about do a pattern match of "XXXX Dataflow:" and put everything in the first custom metrics and process the followings with ":".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
b526bb3
to
dbedbe9
Compare
Client application is responsible for initializing OpenTelemetry to consume these metrics. If OpenTelemetry is not initialized the metrics act as a no-op.