-
Notifications
You must be signed in to change notification settings - Fork 418
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
chore(llmobs): update ragas trace ml app #11952
base: main
Are you sure you want to change the base?
Changes from all commits
f90caa4
5bd3840
0d8f9a6
5f35100
a2580e8
ea038a9
d7f4630
e52f564
74f9b92
3d2a446
2b11d86
fb4bb8c
195cb4a
38e1f62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ | |
from ddtrace.llmobs._constants import INPUT_PARAMETERS | ||
from ddtrace.llmobs._constants import INPUT_PROMPT | ||
from ddtrace.llmobs._constants import INPUT_VALUE | ||
from ddtrace.llmobs._constants import IS_EVALUATION_SPAN | ||
from ddtrace.llmobs._constants import METADATA | ||
from ddtrace.llmobs._constants import METRICS | ||
from ddtrace.llmobs._constants import ML_APP | ||
|
@@ -59,6 +60,7 @@ | |
from ddtrace.llmobs._utils import _get_session_id | ||
from ddtrace.llmobs._utils import _get_span_name | ||
from ddtrace.llmobs._utils import _inject_llmobs_parent_id | ||
from ddtrace.llmobs._utils import _is_evaluation_span | ||
from ddtrace.llmobs._utils import safe_json | ||
from ddtrace.llmobs._utils import validate_prompt | ||
from ddtrace.llmobs._writer import LLMObsEvalMetricWriter | ||
|
@@ -123,16 +125,16 @@ def _submit_llmobs_span(self, span: Span) -> None: | |
"""Generate and submit an LLMObs span event to be sent to LLMObs.""" | ||
span_event = None | ||
is_llm_span = span._get_ctx_item(SPAN_KIND) == "llm" | ||
is_ragas_integration_span = False | ||
is_evaluation_span = False | ||
try: | ||
span_event, is_ragas_integration_span = self._llmobs_span_event(span) | ||
span_event, is_evaluation_span = self._llmobs_span_event(span) | ||
self._llmobs_span_writer.enqueue(span_event) | ||
except (KeyError, TypeError): | ||
log.error( | ||
"Error generating LLMObs span event for span %s, likely due to malformed span", span, exc_info=True | ||
) | ||
finally: | ||
if not span_event or not is_llm_span or is_ragas_integration_span: | ||
if not span_event or not is_llm_span or is_evaluation_span: | ||
return | ||
if self._evaluator_runner: | ||
self._evaluator_runner.enqueue(span_event, span) | ||
|
@@ -185,10 +187,8 @@ def _llmobs_span_event(cls, span: Span) -> Tuple[Dict[str, Any], bool]: | |
metrics = span._get_ctx_item(METRICS) or {} | ||
ml_app = _get_ml_app(span) | ||
|
||
is_ragas_integration_span = False | ||
|
||
if ml_app.startswith(constants.RAGAS_ML_APP_PREFIX): | ||
is_ragas_integration_span = True | ||
is_evaluation_span = _is_evaluation_span(span) | ||
span._set_ctx_item(IS_EVALUATION_SPAN, is_evaluation_span) | ||
Comment on lines
+190
to
+191
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this check in this function scope? |
||
|
||
span._set_ctx_item(ML_APP, ml_app) | ||
parent_id = str(_get_llmobs_parent_id(span) or "undefined") | ||
|
@@ -210,9 +210,9 @@ def _llmobs_span_event(cls, span: Span) -> Tuple[Dict[str, Any], bool]: | |
llmobs_span_event["session_id"] = session_id | ||
|
||
llmobs_span_event["tags"] = cls._llmobs_tags( | ||
span, ml_app, session_id, is_ragas_integration_span=is_ragas_integration_span | ||
span, ml_app, session_id, is_ragas_integration_span=is_evaluation_span | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like we don't necessarily need to pass this information in to |
||
) | ||
return llmobs_span_event, is_ragas_integration_span | ||
return llmobs_span_event, is_evaluation_span | ||
|
||
@staticmethod | ||
def _llmobs_tags( | ||
|
@@ -272,10 +272,6 @@ def _start_service(self) -> None: | |
log.debug("Error starting evaluator runner") | ||
|
||
def _stop_service(self) -> None: | ||
# Remove listener hooks for span events | ||
core.reset_listeners("trace.span_start", self._on_span_start) | ||
core.reset_listeners("trace.span_finish", self._on_span_finish) | ||
|
||
try: | ||
self._evaluator_runner.stop() | ||
# flush remaining evaluation spans & evaluations | ||
|
@@ -290,6 +286,10 @@ def _stop_service(self) -> None: | |
except ServiceStatusError: | ||
log.debug("Error stopping LLMObs writers") | ||
|
||
# Remove listener hooks for span events | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this makes sure hooks aren't removed prematurely while ragas is still running as triggered by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this triggering any bugs? |
||
core.reset_listeners("trace.span_start", self._on_span_start) | ||
core.reset_listeners("trace.span_finish", self._on_span_finish) | ||
|
||
forksafe.unregister(self._child_after_fork) | ||
|
||
@classmethod | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
from ddtrace.llmobs._constants import GEMINI_APM_SPAN_NAME | ||
from ddtrace.llmobs._constants import INTERNAL_CONTEXT_VARIABLE_KEYS | ||
from ddtrace.llmobs._constants import INTERNAL_QUERY_VARIABLE_KEYS | ||
from ddtrace.llmobs._constants import IS_EVALUATION_SPAN | ||
from ddtrace.llmobs._constants import LANGCHAIN_APM_SPAN_NAME | ||
from ddtrace.llmobs._constants import ML_APP | ||
from ddtrace.llmobs._constants import OPENAI_APM_SPAN_NAME | ||
|
@@ -127,6 +128,23 @@ def _get_span_name(span: Span) -> str: | |
return span.name | ||
|
||
|
||
def _is_evaluation_span(span: Span) -> bool: | ||
""" | ||
Return whether or not a span is an evaluation span by checking the span's | ||
nearest LLMObs span ancestor. Default to 'False' | ||
""" | ||
is_evaluation_span = span._get_ctx_item(IS_EVALUATION_SPAN) | ||
if is_evaluation_span is not None: | ||
return is_evaluation_span | ||
llmobs_parent = _get_nearest_llmobs_ancestor(span) | ||
while llmobs_parent: | ||
is_evaluation_span = llmobs_parent._get_ctx_item(IS_EVALUATION_SPAN) | ||
if is_evaluation_span is not None: | ||
return is_evaluation_span | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's do a bool check to be defensive here |
||
llmobs_parent = _get_nearest_llmobs_ancestor(llmobs_parent) | ||
return is_evaluation_span or False | ||
|
||
|
||
def _get_ml_app(span: Span) -> str: | ||
""" | ||
Return the ML app name for a given span, by checking the span's nearest LLMObs span ancestor. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the _ml_obs prefix to make it clear this is coming from us (for future reviewers/users)