-
Notifications
You must be signed in to change notification settings - Fork 478
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
SNOW-1514764: Support Tracing in SPCS runtime #1989
Conversation
|
||
inject(headers) | ||
except ModuleNotFoundError as e: | ||
logger.debug(f"Opentelemtry otel injection failed because of: {e}") |
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.
Not sure if the log is useful or not...
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 think it is better than put a pass
here
@sfc-gh-bdrutu Hi, can you give me a brief explanation why this change has to be in python connector? Is there other ways that we can do this? |
from opentelemetry.propagate import inject | ||
|
||
inject(headers) |
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.
this change would impact general python users, some questions in my head:
- what data is injected into the header?
- this is on the critical path of query execution, what's the overhead of opentelemetry inject added here, e.g., increase in the payload, potential perf regression? also would try-except cause perf regression? I think we need to test perf of this change, and see the payload change.
- what kind of telemetry data are sent to snowflake backend, do we need to consult the legal team about the telemetry?
- will user be aware of this? should we allow customers to opt out open telemetry collection?
- how would backend react to request with new header, do we need any backend change?
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.
@sfc-gh-bdrutu I think some of this question are answered in slack chat, but can you still take a look?
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.
just synced up with @sfc-gh-yuwang , I think once we get perf tested (can be as simple as performing "select 1" for 1 million times) and confirm there is no regression, then let's merge it to unblock SPCS scenario.
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.
what data is injected into the header?
We will add one header "traceparent" which has 55 characters value (always), see https://www.w3.org/TR/trace-context/. So the overhead shouldn't be that significant especially it does not happen if the user doesn't configure opentelemetry. Not sure about performance of try/catch.
what kind of telemetry data are sent to snowflake backend, do we need to consult the legal team about the telemetry?
We are sending the trace/span ids of the notebook operation, again we do this only if the user configures opentelemetry.
will user be aware of this? should we allow customers to opt out open telemetry collection?
User has to import opentelemetry and configure it to have this. This code executes only if the import succeeds which requires user to import and configure opentelemetry.
how would backend react to request with new header, do we need any backend change?
We are taking care of that.
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes #SNOW-1514764
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.
(Optional) PR for stored-proc connector: