Improve Span Links API

[Kludex]

Currently, it's not very ergonomic to create a span link.

Let's assume we have 2 services:

Main Service

from typing import Any

from opentelemetry import trace
from opentelemetry.propagate import extract
from pydantic import TypeAdapter
from redis import Redis

import logfire
import logfire.propagate

logfire.configure(service_name='subscriber')
logfire.instrument_redis()

TA = TypeAdapter[Any](Any)


def main():
    client = Redis()
    logfire.info('Subscriber is running')

    def watch_key(key: str):
        pubsub = client.pubsub()
        pubsub.subscribe(key)

        for message in pubsub.listen():
            if message['type'] == 'message':
                with logfire.span('process_message'):
                    data = TA.validate_json(message['data'])
                    context = extract(data.pop('headers', {}))
                    span = next(iter(context.values()))
                    with logfire.span('process_data', _links=[(span.get_span_context(), None)]):
                        logfire.info(f'Received data: {data}')

    watch_key('key')


main()

And...

Service A

from typing import Any

from pydantic import TypeAdapter
from redis import Redis

import logfire
import logfire.propagate

logfire.configure(service_name='service_a')
logfire.instrument_redis()

TA = TypeAdapter[Any](Any)

client = Redis()


@logfire.instrument()
def service_a():
    logfire.info('Service A is running')

    data: dict[str, Any] = {'data': 'Hello from Service A', 'headers': logfire.propagate.get_context()}
    client.publish('key', TA.dump_json(data))


service_a()

As you see, in the code, we set the headers with logfire.propagate.get_context(), and then on the subscriber, we need to run:

context = extract(data.pop('headers', {}))
span = next(iter(context.values()))
with logfire.span('process_data', _links=[(span.get_span_context(), None)]):
    ...

We can for sure improve this. Either by passing SpanContext to _links, or a Link.

This PR proposes creating:

1. logfire.propagate.build_span_link: Creates a span link from a ContextCarrier, so it makes easier to use the propagate API we created. But... We can also build a span_context, instead of Link itself. It may be a bit more useful. I'm open to rework this.
2. Adds Link and SpanContext to the _links parameter.

---

I need to add tests here. I've first implemented this to bring back the discussion.

• Closes Support for Span link with good DX interface and demo presentation #158

[codecov]

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 99.93%. Comparing base (8434258) to head (89bd667).
Report is 60 commits behind head on main.

Files with missing lines	Patch %	Lines
logfire/propagate.py	50.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              main     #726      +/-   ##
===========================================
- Coverage   100.00%   99.93%   -0.07%     
===========================================
  Files          139      140       +1     
  Lines        11206    11263      +57     
  Branches      1572     1578       +6     
===========================================
+ Hits         11206    11256      +50     
- Misses           0        5       +5     
- Partials         0        2       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alexmojaki]

Why wouldn't the user use with attach_context here?

[Kludex]

If it's attached, then it doesn't make sense to add a span link, does it?

[alexmojaki]

Right, I'm questioning adding (and recommending) build_span_link. In this situation it seems more sensible to use a parent-child relationship instead of a span link.

[Kludex]

I mean, if I change the comment to "later on a task queue", would that be better? Or what you suggest instead?

By the way, I'm not sure build_span_link is the best API for this, if we build_span_context also works. I don't care which if the span(..., _links=[...]) accepts one of them.

[alexmojaki]

I mean, if I change the comment to "later on a task queue", would that be better?

No because regular distributed tracing still seems better.

Or what you suggest instead?

I honestly don't know when span links are useful in general. I suppose in your bigger example there's already a parent process_message. If we assume that for whatever reason we really want to keep that as the parent, then we have to use a link. But then the docs should clarify that so that users only use links when it makes sense.

By the way, I'm not sure build_span_link is the best API for this, if we build_span_context also works. I don't care which if the span(..., _links=[...]) accepts one of them.

I like that idea. Maybe name it get_span_context.

[Kludex]

No because regular distributed tracing still seems better.

You disagree with open-telemetry/opentelemetry-python-contrib#3002 ?

I honestly don't know when span links are useful in general.

Besides Airflow, I'm not sure who uses them... But I feel they are supposed to be useful when you "trigger" something e.g. you have an endpoint, and you trigger a veryyyy long task somewhere else - you want to be able to close the span from the endpoint, and have a link to this very long task.

I like that idea. Maybe name it get_span_context.

👍

[alexmojaki]

No because regular distributed tracing still seems better.

You disagree with open-telemetry/opentelemetry-python-contrib#3002 ?

Overall, yes. What made you post that?

I honestly don't know when span links are useful in general.

Besides Airflow, I'm not sure who uses them... But I feel they are supposed to be useful when you "trigger" something e.g. you have an endpoint, and you trigger a veryyyy long task somewhere else - you want to be able to close the span from the endpoint,

The endpoint span will still be closed. It'll just have children that starts after it ends, which feels weird but should be harmless.

and have a link to this very long task.

The endpoint span won't have a link to the task span, only the other way around. Someone looking at the endpoint span has no easy way to find the task span.

[cloudflare-workers-and-pages]

Deploying logfire-docs with    Cloudflare Pages

Latest commit:	89bd667
Status:	✅  Deploy successful!
Preview URL:	https://3bff861b.logfire-docs.pages.dev
Branch Preview URL:	https://kludex-support-links.logfire-docs.pages.dev

View logs

[alexmojaki]

I mean, if I change the comment to "later on a task queue", would that be better?

No because regular distributed tracing still seems better.

Or what you suggest instead?

I honestly don't know when span links are useful in general. I suppose in your bigger example there's already a parent process_message. If we assume that for whatever reason we really want to keep that as the parent, then we have to use a link. But then the docs should clarify that so that users only use links when it makes sense.

By the way, I'm not sure build_span_link is the best API for this, if we build_span_context also works. I don't care which if the span(..., _links=[...]) accepts one of them.

I like that idea. Maybe name it get_span_context.

[alexmojaki]

The context can contain multiple values, e.g. baggage.

[alexmojaki]

Suggested change

	with logfire.span('process_data', _links=[link]):
	with logfire.span('outer span'):
	# can't use attach_context because we want to keep outer span as the parent
	with logfire.span('process_data', _links=[link]):

[Kludex]

We didn't achieve consensus on span links. There's too much subjectivity involved here.

One conclusion we got is that: we can have the get_span_context method.