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

Improve Span Links API #726

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve Span Links API #726

wants to merge 1 commit into from

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Dec 24, 2024

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.

Copy link

codecov bot commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 56.25000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 99.93%. Comparing base (8434258) to head (16e5534).

Files with missing lines Patch % Lines
logfire/_internal/main.py 60.00% 2 Missing and 2 partials ⚠️
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      139              
  Lines        11206    11218      +12     
  Branches      1572     1574       +2     
===========================================
+ Hits         11206    11211       +5     
- Misses           0        5       +5     
- Partials         0        2       +2     

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

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.

Support for Span link with good DX interface and demo presentation
1 participant