[OPIK-529] Add error_info to spans and traces #885
Conversation
| @@ -732,6 +751,8 @@ private Publisher<? extends Result> insert(List<Span> spans, Connection connecti | |||
| .bind("total_estimated_cost_version" + i, | |||
| estimatedCost.compareTo(BigDecimal.ZERO) > 0 ? ESTIMATED_COST_VERSION : "") | |||
| .bind("tags" + i, span.tags() != null ? span.tags().toArray(String[]::new) : new String[]{}) | |||
| .bind("error_info" + i, | |||
| span.errorInfo() != null ? JsonUtils.readTree(span.errorInfo()).toString() : "") | |||
There was a problem hiding this comment.
Not sure why you need the readTree call here in the write path. It should be enough to just write the errorInfo record.
There are multiple occurrences of this below in this PR.
There was a problem hiding this comment.
ErrorInfo is a record with fixed fields, it's not a JsonNode, so if I write it without JsonUtils, it will be something like this in DB:
ErrorInfo[exceptionType=V_7ivWNh_f, message=zXvv1lMJwe, traceback=M3ZVOqxH0Y]
Then it would be impossible to deserialize and all Clickhouse Json related functions won't work as well.
I chose record Object instead of JsonNode since it will have fixed fileds, not arbitrary Json as metadata.
There was a problem hiding this comment.
@andrescrz we could use here JsonUtils.writeValueAsString instead if you prefer
There was a problem hiding this comment.
After thinking about it twice, I think the most similar approach to other existing Json fields is to convert it to a JsonNode as you're doing. So this is fine.
apps/opik-backend/src/main/java/com/comet/opik/domain/TraceDAO.java
Outdated
Show resolved
Hide resolved
...esources/liquibase/db-app-analytics/migrations/000008_add_error_info_to_spans_and_traces.sql
Outdated
Show resolved
Hide resolved
| @@ -732,6 +751,8 @@ private Publisher<? extends Result> insert(List<Span> spans, Connection connecti | |||
| .bind("total_estimated_cost_version" + i, | |||
| estimatedCost.compareTo(BigDecimal.ZERO) > 0 ? ESTIMATED_COST_VERSION : "") | |||
| .bind("tags" + i, span.tags() != null ? span.tags().toArray(String[]::new) : new String[]{}) | |||
| .bind("error_info" + i, | |||
| span.errorInfo() != null ? JsonUtils.readTree(span.errorInfo()).toString() : "") | |||
There was a problem hiding this comment.
After thinking about it twice, I think the most similar approach to other existing Json fields is to convert it to a JsonNode as you're doing. So this is fine.
apps/opik-backend/src/main/java/com/comet/opik/domain/TraceDAO.java
Outdated
Show resolved
Hide resolved
BorisTkachenko
deleted the
boryst/OPIK-529-error-tracking-be-add-new-error-info-field-to-span-and-trace-models
branch
Details
Add error_info to spans and traces
Testing
Covered by existing tests