Skip to content
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

Support all OTEL keys for instrument suppression #197

Merged
merged 11 commits into from
May 29, 2024

Conversation

jlondonobo
Copy link
Contributor

@jlondonobo jlondonobo commented May 20, 2024

Add support for all OTEL keys for instrument suppression.

This PR adds support for the following keys:

  • "suppress_instrumentation"
  • _SUPPRESS_INSTRUMENTATION_KEY

Comment on lines 201 to 203
@contextmanager
def suppress_instrumentation():
"""Context manager to suppress instrumentation from the LLM client."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some parts of OTEL still use the plain 'suppress_instrumentation' key, so we need to check/set that too, as you said in #135 (comment).

We could maybe upstream a fix to OTEL to consistently use one key, but we need this to also work for older OTEL versions.

Copy link
Member

@adriangb adriangb May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should fix as much of OTEL as we can to make sure it both sets and respects all keys. But yes the unfortunate reality is that both us and OTEL will have to set and check for any keys ever used.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexmojaki before you merge this could you address this comment? I'd like to understand why we can't just use the OTEL function or if we can't because it is missing some functionality I'd like to know that there is a plan to upstream it and then replace our function with theirs or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yes the unfortunate reality is that both us and OTEL will have to set and check for any keys ever used.

I thought we were already in agreement. Similarly, the OTEL util function is somewhat recent, I'd like to not reduce compatibility with older OTEL versions. We especially can't rely on a future version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great so to make sure I understand we agree that OTEL needs to make some changes that will make future versions work correctly because we do not want to bump our minimum version we are going to do the fixes by exposing our own utility functions.

Will our utility functions be able to to just point to the open telemetry ones in the future?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will our utility functions be able to to just point to the open telemetry ones in the future?

Sure, although I think it'll be some time before we decide we don't need a fallback for older OTEL versions.

Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@alexmojaki
Copy link
Contributor

In https://pydanticlogfire.slack.com/archives/C06EDRBSAH3/p1714658053546979 a user essentially requested the suppress_instrumentation function, so I've made it public here.

This PR fixes #135 in the sense that those particular urllib3 logs don't get sent to logfire. They are now logged by the fallback handler of LogfireLoggingHandler which logs to stderr by default. After this, I want to add a snippet like the following to the docs showing how to filter those logs if the noise in the console is still annoying:

import logging
import time
from logging import DEBUG, basicConfig, getLogger

from logfire.integrations.logging import LogfireLoggingHandler

logfire_handler = LogfireLoggingHandler()

# Don't write to stderr the urllib3 debug logs created while exporting traces
urllib3_filter = logging.Filter('urllib3')
logfire_handler.fallback.addFilter(lambda record: not urllib3_filter.filter(record))

basicConfig(handlers=[logfire_handler], level=DEBUG)

logger = getLogger(__name__)

for _ in range(1000):
    logger.info('log')
    time.sleep(0.6)

@alexmojaki
Copy link
Contributor

Thanks @jlondonobo for your help, your changes were pretty much exactly what was needed!

@alexmojaki alexmojaki requested a review from adriangb May 21, 2024 20:05
@alexmojaki alexmojaki enabled auto-merge (squash) May 29, 2024 22:15
@alexmojaki alexmojaki disabled auto-merge May 29, 2024 22:15
@alexmojaki alexmojaki enabled auto-merge (squash) May 29, 2024 22:16
@alexmojaki alexmojaki merged commit f9bb8bc into pydantic:main May 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants