Skip to content

Conversation

@njz-cvm
Copy link
Contributor

@njz-cvm njz-cvm commented Oct 24, 2025

As per #1493, it's current a bit awkward to remove context from instrumentation. This PR adds the "new_context" keyword to ensure designated function calls do not inherit from parent spans.

Here is an example from the new test:

def context_factory():
    return {'new_contex': 123}

@tagged.instrument('hello-world {a=}', record_return=True, new_context=context_factory)
def hello_world(a: int) -> str:
    return f'hello {a}'

@tagged.instrument('parent', record_return=True)
def parent() -> str:
    return hello_world(5)

Alternatively, new_context can be set to True, which is the same as new_context=dict.

@codecov
Copy link

codecov bot commented Oct 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

tagged = logfire.with_tags('test_instrument')

def context_factory():
return {'new_contex': 123}
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't discuss this context factory API, I'm not sure if I like it or understand the point. It's certainly not being tested here, new_contex doesn't do anything or appear in the exported spans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't mention it in the original issue because it wasn't something I'd considered. But when I realized I'd just be using attach_context behind the scenes, it seemed preferable to allow users the option to provide input for attach_context.

If you're against it, it's not that useful for me and I don't mind cutting it. But if it's worth keeping I'll add better testing for it.

Copy link
Contributor

@alexmojaki alexmojaki left a comment

Choose a reason for hiding this comment

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

this feature isn't really compelling enough without the span link

@njz-cvm
Copy link
Contributor Author

njz-cvm commented Oct 27, 2025

this feature isn't really compelling enough without the span link

I'll look at adding it, just trying to determine the best approach.

@alexmojaki
Copy link
Contributor

If you'd still like me to review this, please ensure that all checks are passing.

@njz-cvm njz-cvm requested a review from alexmojaki November 10, 2025 14:37
@njz-cvm
Copy link
Contributor Author

njz-cvm commented Nov 10, 2025

Sorry, I didn't have time to finish the required tests. I have changed the API to be what we previously discussed, with new_context being simply True / False. I agree with you that this makes the most sense.

@alexmojaki alexmojaki requested a review from Copilot December 2, 2025 15:21
@alexmojaki alexmojaki changed the title Add new_context to instrumentation Add new_trace parameter to logfire.instrument Dec 2, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a new_trace parameter to the instrument decorator, enabling instrumented functions to start a new trace instead of being a child of the current span. When new_trace=True, the function creates a new trace with a span link to the parent span, effectively breaking the parent-child relationship while maintaining traceability.

Key Changes

  • Added new_trace boolean parameter to @instrument() decorator
  • New traces include a span link to the previous span context for traceability
  • Implementation uses OpenTelemetry's context manipulation to detach from parent spans

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tests/test_logfire.py Added comprehensive test cases covering new_trace functionality with parent spans, no parent spans, without argument extraction, and with selective argument extraction
logfire/_internal/main.py Updated _fast_span, _instrument_span_with_args, and instrument method signatures to accept and propagate new_trace parameter via **kwargs
logfire/_internal/instrument.py Implemented core new_trace logic using OpenTelemetry trace context management, creating span links and detaching from parent context when enabled

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alexmojaki
Copy link
Contributor

@njz-cvm does this look good to you?

@njz-cvm
Copy link
Contributor Author

njz-cvm commented Dec 2, 2025

Looks great to me, this is a much cleaner implementation.

@alexmojaki alexmojaki merged commit ded46dd into pydantic:main Dec 2, 2025
20 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.

2 participants