Skip to content

Conversation

@avilevy18
Copy link

This PR creates a new set of tests that install the collector on an environment with the config passed in as an environment variable.

b/380100825

Testing on:

  • GCE
  • GKE
  • GKE using the Operator
  • Cloud Run

@avilevy18 avilevy18 force-pushed the avilevy-collector-e2e-poc branch from b605c1b to ed87d23 Compare January 28, 2025 21:31
@avilevy18 avilevy18 marked this pull request as ready for review January 31, 2025 20:26
@avilevy18 avilevy18 requested a review from a team as a code owner January 31, 2025 20:26
@@ -0,0 +1,86 @@
//go:build e2ecollector

package e2etestrunner_collector

Choose a reason for hiding this comment

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

The folder should be named the same as the package, i.e. the folder should be renamed to e2etestrunner_collector

)

func TestMain(m *testing.M) {
rand.New(rand.NewSource(time.Now().UnixNano()))

Choose a reason for hiding this comment

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

Are lines 26..57 the same as in e2etestrunner/main_test.go? Maybe those could be factored out somehow rather than doing the hard duplication like this.

},
logger,
)
if err != nil {

Choose a reason for hiding this comment

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

44..48 is equivalent to

return cleanupTf, err

This comment applies to all the setups here it looks like

// SetupGceCollector Set up the collector to run in GCE container. Creates a new
// GCE VM resources, and runs the specified container image. The
// returned cleanup function tears down the VM.
func SetupGceCollector(

Choose a reason for hiding this comment

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

Do any of these setup functions differ in any way other than the terraform they are running?

now := time.Now()
start := timestamppb.New(now.Add(-window))
end := timestamppb.New(now)
fmt.Println(fmt.Sprintf("resource.type = %q", resourceType))

Choose a reason for hiding this comment

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

Is this println left in intentionally?


fmt.Println(fmt.Sprintf("Entry found: %v", entry))
if err != nil {
t.Errorf(fmt.Sprintf("Could not any logs matching filter: %s", filter))

Choose a reason for hiding this comment

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

  • Should be fatal failure probably?
  • Could not *find any...
  • We might as well include the error in the failure message
Suggested change
t.Errorf(fmt.Sprintf("Could not any logs matching filter: %s", filter))
t.Fatalf(fmt.Sprintf("Could not find any logs matching filter %s: %v", filter, err))

time.Sleep(10 * time.Second)
}

fmt.Println(fmt.Sprintf("Entry found: %v", entry))

Choose a reason for hiding this comment

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

This should be after the err check I think

if tracesFound == 0 {
t.Errorf(fmt.Sprintf("Could not find traces with resource attribute otelcol_google_e2e:%s", args.TestRunID))
}
fmt.Println(fmt.Sprintf("Found traces with otelcol_google_e2e:%s", args.TestRunID))

Choose a reason for hiding this comment

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

Suggested change
fmt.Println(fmt.Sprintf("Found traces with otelcol_google_e2e:%s", args.TestRunID))
fmt.Println(fmt.Sprintf("Found traces with resource attribute otelcol_google_e2e:%s", args.TestRunID))

}

req := cloudService.Projects.Traces.List(args.ProjectID)
req.Filter(fmt.Sprintf("otelcol_google_e2e:%s", args.TestRunID))

Choose a reason for hiding this comment

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

otelcol_google_e2e:%s appears multiple times, maybe the sprintf should be a var that is used in those spots instead.

// limitations under the License.

package e2etestrunner
package util

Choose a reason for hiding this comment

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

We should give this package a clearer name than util, how about e2etesting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants