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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Improve Span Links API #726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16e5534
Improve Span Links API
Kludex Dec 24, 2024
89bd667
Update logfire/_internal/main.py
Kludex Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions logfire/_internal/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import opentelemetry.trace as trace_api
from opentelemetry.metrics import CallbackT, Counter, Histogram, UpDownCounter
from opentelemetry.sdk.trace import ReadableSpan, Span
from opentelemetry.trace import SpanContext, Tracer
from opentelemetry.trace import Link, SpanContext, Tracer
from opentelemetry.util import types as otel_types
from typing_extensions import LiteralString, ParamSpec

Expand Down Expand Up @@ -165,7 +165,7 @@ def _span(
_tags: Sequence[str] | None = None,
_span_name: str | None = None,
_level: LevelName | int | None = None,
_links: Sequence[tuple[SpanContext, otel_types.Attributes]] = (),
_links: Sequence[Link | SpanContext | tuple[SpanContext, otel_types.Attributes]] = (),
) -> LogfireSpan:
try:
stack_info = get_user_stack_info()
Expand Down Expand Up @@ -505,7 +505,7 @@ def span(
_tags: Sequence[str] | None = None,
_span_name: str | None = None,
_level: LevelName | None = None,
_links: Sequence[tuple[SpanContext, otel_types.Attributes]] = (),
_links: Sequence[Link | SpanContext | tuple[SpanContext, otel_types.Attributes]] = (),
**attributes: Any,
) -> LogfireSpan:
"""Context manager for creating a span.
Expand All @@ -524,7 +524,7 @@ def span(
_span_name: The span name. If not provided, the `msg_template` will be used.
_tags: An optional sequence of tags to include in the span.
_level: An optional log level name.
_links: An optional sequence of links to other spans. Each link is a tuple of a span context and attributes.
_links: An optional sequence of links to other spans.
attributes: The arguments to include in the span and format the message template with.
Attributes starting with an underscore are not allowed.
"""
Expand Down Expand Up @@ -1991,13 +1991,22 @@ def __init__(
otlp_attributes: dict[str, otel_types.AttributeValue],
tracer: Tracer,
json_schema_properties: JsonSchemaProperties,
links: Sequence[tuple[SpanContext, otel_types.Attributes]],
links: Sequence[Link | SpanContext | tuple[SpanContext, otel_types.Attributes]],
) -> None:
self._span_name = span_name
self._otlp_attributes = otlp_attributes
self._tracer = tracer
self._json_schema_properties = json_schema_properties
self._links = list(trace_api.Link(context=context, attributes=attributes) for context, attributes in links)

self._links: list[Link] = []
for link in links:
if isinstance(link, Link):
self._links.append(link)
elif isinstance(link, SpanContext):
self._links.append(trace_api.Link(context=link))
else:
context, attributes = link
self._links.append(trace_api.Link(context=context, attributes=attributes))

self._added_attributes = False
self._token: None | object = None
Expand Down
26 changes: 25 additions & 1 deletion logfire/propagate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@
""" # noqa: D205

from contextlib import contextmanager
from typing import Any, Iterator, Mapping
from typing import Any, Iterator, Mapping, cast

from opentelemetry import context, propagate
from opentelemetry.trace import Link, Span

# anything that can be used to carry context, e.g. Headers or a dict
ContextCarrier = Mapping[str, Any]
Expand Down Expand Up @@ -70,3 +71,26 @@
yield
finally:
context.attach(old_context)


def build_span_link(carrier: ContextCarrier) -> Link:
"""Build a span link from a context carrier.

This is useful when you want to link a span in a different service to the current span.

```py
from logfire.propagate import build_span_link, get_context

logfire_context = get_context()

...

# later on in another thread, process or service
link = build_span_link(logfire_context)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why wouldn't the user use with attach_context here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

with logfire.span('process_data', _links=[link]):
Copy link
Contributor

Choose a reason for hiding this comment

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

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]):

...
```
"""
context = propagate.extract(carrier=carrier)
span = cast(Span, next(iter(context.values())))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return Link(span.get_span_context())

Check warning on line 96 in logfire/propagate.py

View check run for this annotation

Codecov / codecov/patch

logfire/propagate.py#L94-L96 

Added lines #L94 - L96 were not covered by tests
Loading