chore(tracing): urllib3 events api migration#17226
Conversation
wconti27
changed the title
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codeowners resolved as |
| :param kwargs: Keyword arguments from the target function | ||
| :return: The ``HTTPResponse`` from the target function | ||
| """ | ||
| if not tracer.enabled and not asm_config._apm_opt_out: |
There was a problem hiding this comment.
we probably don't want this?
since we want to create instrumentation hook points in the integration no matter what, they will just be no-op if tracing/asm/ aren't doing anything.
baking the logic for all possible products which may need this information into each integration is a coupling we likely want to avoid.
| ), | ||
| ) | ||
| # Set integration_config in context data so _start_span and _finish_span can find it. | ||
| # This is needed because getattr(event, "config", None) may return None on Python 3.9 |
There was a problem hiding this comment.
why?
config.urllib3 is already attached to the event... so... why is this needed at all? seems like a waste
| ) -> None: | ||
| event: HttpClientRequestEvent = ctx.event | ||
| # Prefer event.config, fall back to context data for Python 3.9 compatibility | ||
| integration_config = getattr(event, "config", None) or ctx.get_item("integration_config") |
There was a problem hiding this comment.
given we always set integration_config right now, then why even have event.config?
also, we are typed as HttpClientRequestEvent, so we know the config attribute exists? so why the getattr at all?
|
if we are going to do some decent sized refactors of integration, we should also enable type checking for them Lines 54 to 55 in 5a86489 |
2 participants
Description
Migrates Urllib3 to Events API
Testing
Risks
Additional Notes