Write RFC to add Span Links to MLFlow#6
Conversation
khaledsulayman
force-pushed
the
add-otel-spanlink
branch
from
3ad4ff8 to
13a108b
Compare
|
|
||
| ```python | ||
| for link in span_a_links: | ||
| span_b.add_link(link) |
There was a problem hiding this comment.
This API should be sufficent to cover @mlflow.trace. Users can call mlflow.get_current_active_span() to fetch the span object inside the function.
There was a problem hiding this comment.
This is true. However, OTel prefers linking at span start-time vs. after. The main reason for this is head sampling, i.e. links are needed at span start time so _MlflowSampler can catch them.
It's debatable whether or not this is worth the extra complexity.
There was a problem hiding this comment.
https://opentelemetry.io/docs/languages/dotnet/traces/links-based-sampler/ worth considering if we want to enable links-based sampling in the future.
There was a problem hiding this comment.
Ah I see, thanks for the clarification! I'm fine with starting with limited set of apis to keep sampling logic simple.
There was a problem hiding this comment.
Sounds good! I'll keep it in the "maybe" list of entry points and during development we can see in practice if it's worth the extra complexity.
|
@khaledsulayman Thanks for the proposal! Left a few comments |
Signed-off-by: Khaled Sulayman <ksulayma@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
khaledsulayman
force-pushed
the
add-otel-spanlink
branch
from
13a108b to
4c33b0b
Compare
|
@B-Step62 Scoped the |
B-Step62
left a comment
B-Step62
left a comment
There was a problem hiding this comment.
Looks great once #6 (comment) is addressed!
Signed-off-by: Khaled Sulayman <ksulayma@redhat.com>
Signed-off-by: Khaled Sulayman <ksulayma@redhat.com>
|
Addressed! Also updated the name because an RFC 0002 has already merged. |
|
@B-Step62 are we good to merge? Or do we need another review? |
|
@khaledsulayman Yes, please feel free to go ahead to the PR! |
2 participants
Proposal for adding support for Span Links, an OTel primitive. This was requested in mlflow/mlflow#21025