feat: allow users to stop a running test #2367

schoren · 2023-04-11T19:00:27Z

This PR implements the stop running request feature. The endpoint was already mocked, but the URL was inconistent with the rest of the tests urls.

This still needs some FE work. I was able to make the web not crash with the new stopped run state, but the UI doesn’t show the trace once stopped, even when it is saved to the run info. Also we probably need a button or something.

Fixes

[Error Handling] - Backend - Stopping a Test Run #2183

Checklist

tested locally
added new dependencies
updated the docs
added a test

schoren · 2023-04-11T19:00:58Z

api/openapi.yaml

@@ -825,7 +825,7 @@ paths:
            application/yaml:
              schema:
                type: string
-  /test/{testId}/run/{runId}/stop:
+  /tests/{testId}/run/{runId}/stop:


all test endpoints are actually /tests with a trailing s

schoren · 2023-04-11T19:01:36Z

local-config/collector.config.yaml

@@ -13,7 +13,7 @@ exporters:
    loglevel: debug

  otlp/1:
-    endpoint: host.docker.internal:21321
+    endpoint: tracetest:21321


this was probablly a leftover from someone doing docker tests, it should point to the internal url

Question: is this file intended just for local testing, or do we use it in some pipelines?

local testing. It's used when you run the ./run.sh script for example, that is mainly targetted to run an env similar to CI, so you can run the tracetesting tests locally easily.

schoren · 2023-04-11T19:01:54Z

local-config/tracetest.provision.yaml

@@ -4,3 +4,12 @@ spec:
  name: OpenTelemetry Collector
  type: otlp
  isdefault: true
+---
+type: Demo


addin a default demo locally eased the testing process

schoren · 2023-04-11T19:04:05Z

server/executor/runner.go

-	ctx                 context.Context
-	test                model.Test
-	run                 model.Run
-	subscriptionManager *subscription.Manager


this var was unused

schoren · 2023-04-11T19:04:32Z

server/executor/runner.go

@@ -132,14 +131,18 @@ func getNewCtx(ctx context.Context) context.Context {
 }

 func (r persistentRunner) Run(ctx context.Context, test model.Test, metadata model.RunMetadata, environment model.Environment) model.Run {
-	ctx = getNewCtx(ctx)
+	ctx, cancelCtx := context.WithCancel(


we want to make the context cancellable, since it isn't by default

schoren · 2023-04-11T19:05:17Z

server/executor/trace_poller.go

+	select {
+	default:
+	case <-job.ctx.Done():
+		fmt.Println("trace poller. Context cancelled.")
+	}
+


Handle context cancelation

schoren · 2023-04-11T19:05:59Z

server/executor/trace_poller.go

@@ -190,7 +196,13 @@ func (tp tracePoller) runAssertions(job PollingRequest) {
 func (tp tracePoller) handleTraceDBError(job PollingRequest, err error) (bool, string) {
 	run := job.run

-	pp := *tp.ppGetter.GetDefault(job.ctx).Periodic
+	profile := tp.ppGetter.GetDefault(job.ctx)


sometimes the context can get cancelled in the middle of this func call, causing a nil pointer panic. This new approach handles the case more gracefully

jorgeepc

We need to update the OpenAPI specs to include the new run state

tracetest/api/tests.yaml

Line 124 in b4f9814

enum:

schoren · 2023-04-11T20:02:25Z

@jorgeepc fixed!

server/executor/run_stop.go

danielbdias · 2023-04-11T20:27:16Z

server/executor/trace_poller.go

+	select {
+	default:
+	case <-job.ctx.Done():
+		fmt.Println("trace poller. Context cancelled.")


Do you think we should do an early return here in case of cancelation? Or do we need to execute the Job process even after receiving the cancel signal?

danielbdias · 2023-04-11T20:30:23Z

server/executor/runner.go

Something to think about in another PR: is there a way to add a unit test that checks if the runner behaved correctly on a cancel case?

schoren · 2023-04-11T21:57:02Z

server/subscription/manager.go

@@ -4,13 +4,13 @@ import "sync"

 type Manager struct {
 	subscriptions map[string][]Subscriber
-	mutex         sync.Mutex
+	mutex         sync.RWMutex


All changes in this file are to fix the race condition making some tests fail

schoren · 2023-04-11T21:57:38Z

server/executor/eventemitter.go

-	em.mutex.Lock()
-	defer em.mutex.Unlock()
-


this lock is moved to the publisher now

schoren · 2023-04-11T21:58:34Z

server/executor/transaction_runner_test.go

@@ -32,26 +32,27 @@ func (r *fakeTestRunner) Run(ctx context.Context, test model.Test, metadata mode
 	}

 	go func() {
+		run := newRun                      // make a local copy to avoid race conditions


by using a copy instead of the original var, we avoid race conditions

schoren · 2023-04-11T21:59:33Z

server/executor/transaction_runner_test.go

-	<-done
-
-	assert(t, transactionRun)
+	done := make(chan model.TransactionRun, 1)


instead of using the channel as a done signal, we use it to return the relevant value. by not sharing a variable between 2 goroutines, we eliminate a race condition

schoren · 2023-04-11T21:59:47Z

server/executor/transaction_runner_test.go

+	case finalRun = <-done:
+		subscriptionManager.Unsubscribe(transactionRun.ResourceID(), sf.ID()) //cleanup to avoid race conditions
+		fmt.Println("run completed")
+	case <-time.After(10 * time.Second):


also it allows to set a timeout instead of waiting forever

danielbdias · 2023-04-11T23:08:24Z

server/subscription/manager.go

+	m.mutex.RLock()
+	defer m.mutex.RUnlock()


Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com>

schoren commented Apr 11, 2023

View reviewed changes

schoren requested review from jorgeepc, xoscar and danielbdias April 11, 2023 19:09

jorgeepc reviewed Apr 11, 2023

View reviewed changes

danielbdias approved these changes Apr 11, 2023

View reviewed changes

schoren linked an issue Apr 11, 2023 that may be closed by this pull request

[Error Handling] - Backend - Stopping a Test Run #2183

Closed

schoren force-pushed the add-stop-test-feature branch from 29ab437 to 8b38d1e Compare April 11, 2023 21:08

schoren requested review from danielbdias and jorgeepc April 11, 2023 21:56

schoren commented Apr 11, 2023

View reviewed changes

danielbdias reviewed Apr 11, 2023

View reviewed changes

server/subscription/manager.go Outdated

Comment on lines 51 to 52

m.mutex.RLock()

defer m.mutex.RUnlock()

Copy link

Contributor

danielbdias Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

schoren and others added 15 commits April 13, 2023 12:04

fix endpoint url

76fedd9

enable demo for local runs

d4955bd

fix local collector config

054235d

imlement stop

405fad4

refactor

24dbe49

remove unsubscribe line

435745f

add constant

d12b71a

Update server/executor/run_stop.go

f6ffa52

Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com>

Update server/executor/run_stop.go

16cc9ea

Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com>

improve handling of cancelled context

b7d4978

test

ab387f9

fix race conditions

8a1939a

lower timeout

687cbfb

tmp disable cleanup step

3c5f243

add debut port checker

2fa05e7

schoren added 2 commits April 13, 2023 12:04

test

68efd30

test

2464636

schoren force-pushed the add-stop-test-feature branch from 4bfd856 to 2464636 Compare April 13, 2023 15:04

test

6ffc2dc

schoren merged commit ba47e42 into main Apr 13, 2023
26 checks passed

schoren deleted the add-stop-test-feature branch April 13, 2023 15:31

jorgeepc mentioned this pull request Apr 13, 2023

[Error Handling] - Add calls to the EventEmitter on Trace fetching (STOPPED_INFO event) #2290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow users to stop a running test #2367

feat: allow users to stop a running test #2367

schoren commented Apr 11, 2023 •

edited

schoren Apr 11, 2023

schoren Apr 11, 2023

danielbdias Apr 11, 2023

schoren Apr 11, 2023

schoren Apr 11, 2023

schoren Apr 11, 2023

schoren Apr 11, 2023

schoren Apr 11, 2023

schoren Apr 11, 2023

jorgeepc left a comment

schoren commented Apr 11, 2023

danielbdias Apr 11, 2023

danielbdias Apr 11, 2023

schoren Apr 11, 2023

schoren Apr 11, 2023

schoren Apr 11, 2023

schoren Apr 11, 2023

schoren Apr 11, 2023

danielbdias Apr 11, 2023

feat: allow users to stop a running test #2367

feat: allow users to stop a running test #2367

Conversation

schoren commented Apr 11, 2023 • edited

Fixes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgeepc left a comment

Choose a reason for hiding this comment

schoren commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schoren commented Apr 11, 2023 •

edited