-
Notifications
You must be signed in to change notification settings - Fork 675
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
fix(sdk): support a "block-list" of things to not instrument #1958
fix(sdk): support a "block-list" of things to not instrument #1958
Conversation
One more note, if someone specifically asks for requests package instrument, I'm not considering the disable behavior. |
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! I think there's a better way to do it
@@ -62,6 +62,7 @@ class TracerWrapper(object): | |||
def __new__( | |||
cls, | |||
disable_batch=False, | |||
disable_requests_instrumentation=True, |
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 seems highly specific, don't you think? I think I'd rather add something like the "instruments" parameter but that can be sort of a "block list" where any of the libraries in that parameter won't be instrumented.
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.
Block list sounds like a better idea. The reason I went specific was because the tracer is normally expected to instrument GenAI-related packages, but has requests
as a special additional scenario. Block list would be broad enough for future developments as well.
requests
package by defaultinit_marqo_instrumentor() | ||
init_lancedb_instrumentor() | ||
init_groq_instrumentor() | ||
def init_instrumentations(should_enrich_metrics: bool, block_instruments: Optional[Set[Instruments]] = None): |
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.
ideally this might be an opportunity to combine these 2 ways and just have init_instrumentations
work the same as the if instruments is not None
clause - just set all instrumentations as the value of instruments
. WDYT?
@@ -44,6 +44,7 @@ def init( | |||
api_key: str = None, | |||
headers: Dict[str, str] = {}, | |||
disable_batch=False, | |||
disable_requests_instrumentation: bool = True, |
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.
remove?
7c830ba
to
bf4c0c1
Compare
bf4c0c1
to
5607c9b
Compare
I have added tests that cover my changes.
Need help here. There are no specific tests checking for
requests
. Where should I add the test?If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
Could you give a recommendation on how to do this with our
HoneyHiveTracer
?Is there a particular example script that would work?
feat(instrumentation): ...
orfix(instrumentation): ...
.TBD, once the tests are running