-
Notifications
You must be signed in to change notification settings - Fork 71
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
Adding PollingEvents #2291
Adding PollingEvents #2291
Conversation
func TraceQueuedInfo(testID id.ID, runID int) model.TestRunEvent { | ||
return model.TestRunEvent{ | ||
TestID: testID, | ||
RunID: runID, | ||
Stage: model.StageTrace, | ||
Type: "QUEUED_INFO", | ||
Title: "Trace Run has been queued to start the fetching process", | ||
Description: "Trace Run has been queued to start the fetching process", | ||
CreatedAt: time.Now(), | ||
DataStoreConnection: model.ConnectionResult{}, | ||
Polling: model.PollingInfo{}, | ||
Outputs: []model.OutputInfo{}, | ||
} | ||
} |
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.
This event can be captured by the TracePollingStart
because they are the same. Thinking about that, I've removed this event and focused on sending the TracePollingStart
event.
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.
Looks great @danielbdias I just want to understand if we are going to execute a Data Store Connection Test
at any point (I see you are removing the TraceDataStoreConnectionInfo
event) Not a blocker for this PR :)
@@ -61,10 +61,9 @@ func (e TestRunEvent) ResourceID() string { | |||
} | |||
|
|||
type PollingInfo struct { | |||
Type PollingType | |||
ReasonNextIteration string |
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.
We need to change this field to the TracePollingSuccess
event, because we have reason to stop the iterations but not to run a new one.
For instance: we can stop a polling process because the trace hasn't changed for a while or due to a timeout. But we cannot say that about the iteration. Does it make sense?
@jorgeepc Thinking for a while and seeing the specs, it makes sense to have a |
@@ -98,17 +102,46 @@ func (pe DefaultPollerExecutor) ExecuteRequest(request *PollingRequest) (bool, m | |||
return false, model.Run{}, err | |||
} | |||
|
|||
if request.IsFirstRequest() { | |||
connectionResult := traceDB.TestConnection(request.ctx) |
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.
Team, a philosophical question here: it makes sense to stop the polling process here if we receive a connection error on this test?
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 think so. Unless we define a retry strategy here, otherwise small network communication problems might cause the trace to not be fetched.
server/executor/poller_executor.go
Outdated
anotherErr := pe.eventEmitter.Emit(request.ctx, events.TraceFetchingError(request.test.ID, request.run.ID, err)) | ||
if anotherErr != nil { | ||
log.Printf("[PollerExecutor] Test %s Run %d: failed to emit TraceFetchingError event: error: %s\n", request.test.ID, request.run.ID, anotherErr.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.
A would expect a connection test to happen here, so if it fails, we at least know if the data store connection is working. We might want to filter out the ErrTraceNotFound
error though.
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.
Have you considered this connection a complement of the TraceFetchingError
event? Something like this:
if err != nil {
connectionResult := traceDB.TestConnection(request.ctx)
anotherErr := pe.eventEmitter.Emit(request.ctx, events.TraceFetchingError(request.test.ID, request.run.ID, connectionResult, err))
if anotherErr != nil {
log.Printf("[PollerExecutor] Test %s Run %d: failed to emit TraceFetchingError event: error: %s\n", request.test.ID, request.run.ID, anotherErr.Error())
}
}
6dce506
to
70a3bea
Compare
70a3bea
to
53f60f2
Compare
This PR aims to add polling events to a test run.
Fixes
Checklist