-
Notifications
You must be signed in to change notification settings - Fork 477
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-1763555: OpenTelemetry integration utterly breaks driver if using DataDog #2084
Comments
This was directly caused by #1989 |
specifically this is the erroneous assumption, there are many ways the import can fail. |
affected by this as well |
Seems like a straightforward issue to fix... Can we get some eyes on this one? |
thanks folks for drawing attention to this, we'll take a look. |
Thanks for your bug report, this is very valuable input. Unfortunately, after further investigation this proves to be an issue specific to DataDog because by just including a clean This is still a problem that Snowflake can fix by catching all the exceptions there, though that is not the right way of writing code, since you cannot have a catch all exception every time you call any API that should not return an exception, but Snowflake will do it anyway since tracing should not break your data aplication. |
@sfc-gh-bdrutu Based on what you've said, I've taken the liberty of opening an issue on the ddtrace-py repository- DataDog/dd-trace-py#11407 |
Based on my experience with OpenTelemetry in different environments, OpenTelemetry was designed with flexibility in mind, supporting scenarios where telemetry is directly embedded within libraries, rather than requiring separate instrumentation libraries. |
I'm facing this as well. Specifically, incompatible with https://github.com/DataDog/datadog-lambda-python/releases/tag/v6.99.0 |
@bogdandrutu you literally have the same bio on your two accounts. |
Since I was unable to reproduce the issue I'd really like someone with the issue to test drive the change I proposed in #2106 . |
merging the PR autoclosed this issue, however it needs a release first, so reopened until release is out. |
released in PythonConnector v3.12.4 |
We're getting a ton of log spam due to this.
Which it appears is due to datadog setting up a different context provider for if someone enables their opentelemetry integration. Please implement opentelemetry in a way that it can be disabled with an environment variable, or put the functionality into a separate package. |
Hi @lattwood You should talk to the DD folks about this problem since our usage is a legit and correct usage of the opentelemetry-api.
Talk to them maybe they shouldn't do that, or they should correctly do it.
We are not "implementing" opentelemetry, we are only propagate the context to Snowflake and as mentioned our usage is correct and is DD causing your issue. |
Python version
N/A
Operating system and processor architecture
N/A
Installed packages
What did you do?
I used the driver w/ Datadog APM, which installs the opentelemetry packages that are used as the check to see if we can inject trace context.
With DataDog, this gets happens on every query, and it fails to run.
This is because the only exception caught is ModuleNotFoundError, and opentracing is throwing a ValueError which isn't getting caught.
(please forgive any typos, I copied the backtrace from a screenshot)
What did you expect to see?
I would expect that opentelemetry integration would be done via this- https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/instrumentation
and not via a hack in the actual library that breaks the driver for anyone that has opentelemetry installed but not in-use.
I hope that this issue results in that change being reverted, and the original requestor of the feature gets this done properly in the open-telemetry project.
Can you set logging to DEBUG and collect the logs?
We already have it at DEBUG and that's the problem.
The text was updated successfully, but these errors were encountered: