Backend: Refactor trace spans (query building + response processing) #257
Conversation
idastambuk
force-pushed
the
refactor-trace-spans
branch
from
b8327cd
to
1fbb42b
Compare
idastambuk
force-pushed
the
refactor-trace-spans
branch
from
1fbb42b
to
c95e963
Compare
| @@ -188,8 +188,7 @@ function getStackTraces(events: OpenSearchSpanEvent[]): string[] | undefined { | |||
| const stackTraces = events | |||
| .filter(event => event.attributes.error) | |||
| .map(event => `${event.name}: ${event.attributes.error}`); | |||
| // if we return an empty array, Trace panel plugin shows "0" | |||
| return stackTraces.length > 0 ? stackTraces : undefined; | |||
| return stackTraces; | |||
There was a problem hiding this comment.
fixed this in the Trace plugin a while ago, so Im updating this.
idastambuk
requested review from
iwysiu,
kevinwcyu and
fridgepoet
and removed request for
a team
idastambuk
force-pushed
the
refactor-trace-spans
branch
from
e9abe43
to
68ebbe0
Compare
There was a problem hiding this comment.
The changes look good! I added a few more small comments.
As a heads up, if you rebase and force push a PR that already has comments on it, it messes up the history and breaks the links for all the old comments, which makes it harder to review. You can do a merge commit instead to fix the merge conflicts.
pkg/opensearch/client/models.go
Outdated
| // QueryStringFilter represents a query string search filter | ||
| type QueryStringFilter struct { | ||
| Filter | ||
| Query string | ||
| AnalyzeWildcard bool | ||
| } | ||
|
|
||
| func (m MustTerm) MarshalJSON() ([]byte, error) { | ||
| root := map[string]interface{}{} | ||
| if m.Term != nil && m.Term.TraceId != "" { |
There was a problem hiding this comment.
I don't think it necessarily make sense here (because of the possibility of Term being non-nil but TraceId being "") but as a fun fact, you can use the omitEmpty json flag to skip empty fields and set json names without having to write a custom 'MarshalJSON' function
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func Test_trace_spans_request(t *testing.T) { |
There was a problem hiding this comment.
The comment looks good! This file should have the same comment as lucene_trace_list_test.go
| Key string | ||
| Value any | ||
| } | ||
| func sortObjectsByKey(rawObject *data.Field) []KeyValue { |
There was a problem hiding this comment.
Instead of having panics in these helper functions, you can pass them a testing.T and call require in them. (You can mark them as helper functions with the line t.Helper() at the start of the function and test errors will be returned as the line that calls the function)
| if jsonRawMessage == nil { | ||
| panic("json.RawMessage is nil") | ||
| } | ||
|
|
There was a problem hiding this comment.
Deeply obnoxious style nit: To keep variables/requires in relevant code blocks, this could be:
jsonRawMessage, ok := rawObject.At(0).(*json.RawMessage)
require.True(t, ok)
require.NotNil(t, jsonRawMessage)
var sortedObject []KeyValue
err := json.Unmarshal(*jsonRawMessage, &sortedObject)
require.Nil(t, err)
(same for sortLogsByTimestamp)
idastambuk
changed the title
What this PR does / why we need it:
Implements #223
Support for trace spans on the backend, only in Explore, to follow our rollout stage plan.
Which issue(s) this PR fixes:
Fixes #223
Special notes for your reviewer:
I couldn't get the snapshot test to work, because in response processing I worked with a lot of maps, which makes the output not follow consistent order of fields. This doesn't matter when it comes to the trace visualization, as it needs specific named fields to display different parts of the UI. Therefore I decided I wouldn't change the original code in order to comply with tests (like sorting fields etc), but added a few utils int he tests to just check that all the fields are in the output, wherever.