-
Notifications
You must be signed in to change notification settings - Fork 148
feat(csharp/src/Drivers/Databricks): Implement Telemetry Reporting for Databricks #3191
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
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,73 @@ | |||
/* | |||
* Licensed to the Apache Software Foundation (ASF) under one or more |
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.
please add a readme file in this folder, on how is the file generated.
csharp/src/Drivers/Databricks/Telemetry/DatabricksActivityListener.cs
Outdated
Show resolved
Hide resolved
@birschick-bq Could you take a look at this PR? Especially with the logic in DatabricksActivityListener.cs. |
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.
Thanks.
This is a start. I'll spend some more time on Wednesday to review.
csharp/src/Drivers/Databricks/Telemetry/DatabricksActivityListener.cs
Outdated
Show resolved
Hide resolved
csharp/src/Drivers/Databricks/Telemetry/DatabricksActivityListener.cs
Outdated
Show resolved
Hide resolved
csharp/src/Drivers/Databricks/Telemetry/DatabricksActivityListener.cs
Outdated
Show resolved
Hide resolved
csharp/src/Drivers/Databricks/Telemetry/DatabricksActivityListener.cs
Outdated
Show resolved
Hide resolved
csharp/src/Drivers/Databricks/Telemetry/DatabricksActivityListener.cs
Outdated
Show resolved
Hide resolved
csharp/src/Drivers/Databricks/Telemetry/DatabricksActivityListener.cs
Outdated
Show resolved
Hide resolved
P.S. You should normally work on a feature branch and not |
Thanks for your review. Please take a look again when you get the chance.
I see, is there a need to replace this PR? |
@jeremytang-db - I'm working on a change to allow the |
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.
Thanks for this large change! Can you please resolve the merge conflict (perhaps by rebasing)? And is there a specific reason for all the added types to be public
? If not, could you please make them internal
instead? They seem like the kind of implementation details that shouldn't be part of a public contract.
@CurtHagenlocher @birschick-bq @jadewang-db |
{ | ||
_httpClient = httpClient; | ||
_accessToken = accessToken; | ||
_telemetryUrl = !string.IsNullOrEmpty(hostUrl) ? accessToken != null ? $"https://{hostUrl}/telemetry" : $"https://{hostUrl}/telemetry-unauth" : null; |
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.
I don't think this should be included with the driver. We have OpenTelemetry implemented, and the way to get this is via the OTEL logger. This is basically forcing extra traffic (especially on the service side) that isn't needed.
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.
Oh I was unaware of this. Can you point me to how to access the OTEL logger? Thank you!
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.
Hi Jeremy. Thanks. Here are a few more comments.
csharp/src/Drivers/Databricks/Telemetry/DatabricksActivityListener.cs
Outdated
Show resolved
Hide resolved
@jeremytang-db - I see you updated the comments, but I don't see any new pushed commits. Do you any commits that aren't pushed yet? Is it on another branch? |
@birschick-bq Sorry, I pushed to the wrong branch. It should be updated now. |
The subject of client-side telemetry is often a contentious one. I think there should be an opt-out for it on the connection string. Unrelated to that, there may have been a merge or rebase error as this doesn't currently appear to build. |
Created Telemetry models to fit Telemetry data in required format.
Uses ActivityListener to collect activities.
Still need to implement and check flag to enable and disable. Should be disabled by default.