-
Notifications
You must be signed in to change notification settings - Fork 73
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
feat: add sumo logic datastore #3428
Conversation
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.
Hey @mathnogueira I added two comments, let me know what you think
@@ -145,6 +145,16 @@ func (w *tracePollerEvaluatorWorker) ProcessItem(ctx context.Context, job execut | |||
return | |||
} | |||
|
|||
if augmenter, ok := traceDB.(tracedb.TraceAugmenter); ok { |
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.
what is this about? was this needed for sumologic?
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.
Sumo Logic Trace API returns trace spans without attributes. I believe that this method is used to augment the trace and fetch another API that returns the span attributes.
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.
Is there a reason why this needs to happen in a different step? i.e not in the same step as the trace poller?
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.
I just added a detailed comment on why this should be here and not in the other step.
server/tracedb/sumologic.go
Outdated
} | ||
|
||
// GetTraceByID implements TraceDB. | ||
func (db *sumologicDB) GetTraceByID(ctx context.Context, traceID string) (traces.Trace, error) { |
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.
I don't see a test connection function, maybe I'm missing something?
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.
Forgot about it. Just added it
server/tracedb/sumologic.go
Outdated
@@ -0,0 +1,296 @@ | |||
package tracedb |
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.
Question: don't we need an implementation like this for the Agent? We need to implement something on https://github.com/kubeshop/tracetest/tree/main/agent/workers/datastores, no?
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.
Also, it would be good to have a unit test for SumoLogic on the RM's Provisioner, here: https://github.com/kubeshop/tracetest/blob/main/server/provisioning/provisioning_test.go#L350
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.
The FE code looks good to me! we just missing the icon right?
@xoscar icon added |
This PR adds the implementation for the new
Sumo Logic
datastore. Related to issue #3423Checklist
Loom video
https://www.loom.com/share/c1a5b5bb40fb4100bde6ed48dce0e9f5
Settings page