Skip to content
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

[chore] Enable all (passing) goleak checks #30451

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

crobert-1
Copy link
Member

@crobert-1 crobert-1 commented Jan 11, 2024

Description:

This PR contains 3 main changes:

  1. Make a copy of the same file into every package whose goleak check is currently passing. The only change for the package_test.go file in each package is the package name itself.
  2. Rename processor/tailsamplingprocessor/main_test.go to package_test.go for uniformity. package_test.go is the name we chose for core (see discussion here).
  3. Run make gotidy to add goleak dependency.

I included the script used to make these changes in the issue.

Link to tracking Issue:
#30438

Testing:
Ran tests in every referenced package, they're passing.

@dmitryax
Copy link
Member

It would be great to generate this test along with #27849. If we need to skip it for some packages we can add skip_goleak: true with a comment to the tests section metadata.yaml

@crobert-1
Copy link
Member Author

It would be great to generate this test along with #27849. If we need to skip it for some packages we can add skip_goleak: true with a comment to the tests section metadata.yaml

Would it be alright if I open an issue for this and add it as a work item to issue #30438?

I have some questions about how that would work, and I'm thinking it would be good to keep a PR this big to just copy changes. I worry that introducing functionality changes would make it that much worse to review. Also, if these files look good to reviewers, they shouldn't change if being generated by mdatagen (so this PR's changes are still valid and won't be overwritten later).

@dmitryax
Copy link
Member

Sounds good to me. 👍 We can do it as a follow-up

@crobert-1
Copy link
Member Author

I've filed #30483 for using mdatagen to generate these files instead 👍

@crobert-1
Copy link
Member Author

Apologies for all of the updates, it looks like over the last few days a lot of packages have added opencensus as an indirect dependency, causing them to fail goleak checks. I'm just going to do this whole PR over again and post the removals in one fell swoop at this point, rather than updating one at a time.

@crobert-1
Copy link
Member Author

This should be ready to review again. I apologize for the size.

For easy reviewing, you can view the output of the following command I ran against this branch to confirm that it's just the same thing copied in many places.

$ cat $(git diff-tree --no-commit-id --name-only -r HEAD..origin/main | grep -v "go\..*") | sort | uniq

Here's each part explained:

  1. git diff-tree --no-commit-id --name-only -r HEAD..origin/main - Get the names of files that have been changed in the current branch (files that are different than origin/main).
  2. grep -v "go\..*" - Ignore go.mod and go.sum. This could be grep -v "go.mod" | grep -v "go.sum" if it's more clear.
  3. cat $() - Display all of the contents of the changed files.
  4. sort | uniq- Only show unique output, sort must be used as uniq only compares lines next to each other. The output shows the only things that have changed are package names, and the basic structure is the same. Note that sort means output lines are in alphabetical order, not what the file itself looks like.
cat: processor/tailsamplingprocessor/main_test.go: No such file or directory # As noted in description, this file was renamed.

	"go.uber.org/goleak"
	"testing"
	goleak.VerifyTestMain(m)
	goleak.VerifyTestMain(m, goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start")) # This is from the renamed tailsamplingprocessor file.
)
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0
func TestMain(m *testing.M) {
import (
package action
package add
package alias
package attraction
package awsecscontainermetrics
package awsmsk
package awsutil
package azure
package batch
package batchperresourceattr
package batchpersignal
package cache
package checkpoint
package chrony
package cluster
package common
package compress
package consul
package consumerretry
package containerinsight
package converter
package copy
package correlations
package csv
package ctimefmt
package cwlogs
package cwmetricstream
package docker
package dpfilters
package drop
package ec2
package ecs
package ecsmock
package emittest
package endpoints
package entry
package errctx
package errors
package experimentalmetricmetadata
package extractors
package file
package fileconsumer
package filetest
package filter
package filterconfig
package filterexpr
package filterlog
package filtermatcher
package filtermetric
package filterset
package filterspan
package finder
package fingerprint
package flatten
package flush
package gcp
package generate
package golden
package goldendataset
package handlecount
package handler
package header
package idbatcher
package idutils
package internal
package jaeger
package journald
package json
package k8s
package k8snode
package k8sutil
package kafka
package key
package keyvalue
package kubelet
package logs
package loki
package maps
package matcher
package metadata
package metrics
package metricstestutil
package migrate
package model
package move
package namedpipe
package noop
package objmodel
package observer
package opencensus
package openshift
package operator
package ottldatapoint
package ottllog
package ottlmetric
package ottlresource
package ottlscope
package ottlspan
package ottlspanevent
package ottltest
package pdatautil
package perfcounters
package pipeline
package plogtest
package pmetrictest
package producer
package prometheus
package prometheusremotewrite
package protocol
package ptracetest
package reader
package regexp
package remove
package requestcounter
package resourcetotelemetry
package retain
package router
package s3provider
package sanitize
package scanner
package scope
package scrub
package serialization
package service
package severity
package signalfx
package sketches
package skywalking
package source
package split
package splittest
package stdin
package stdout
package storage
package store
package strict
package supervisor
package syslog
package system
package tailsamplingprocessor
package tcp
package telemetrytest
package tenant
package testbed
package testutil
package textutils
package time
package timeutils
package tools
package trace
package traces
package tracesegment
package tracetracker
package traceutil
package tracking
package translation
package translator
package transport
package trie
package trim
package ucal
package udp
package unmarshalertest
package unquote
package uri
package util
package utils
package valid
package windows
package winperfcounters
package zipkinv1
package zipkinv2
}

@dmitryax dmitryax merged commit 0f92b21 into open-telemetry:main Jan 17, 2024
85 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 17, 2024
mfyuce pushed a commit to mfyuce/opentelemetry-collector-contrib that referenced this pull request Jan 18, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
This PR contains 3 main changes:
1. Make a copy of the same file into every package whose `goleak` check
is currently passing. The only change for the `package_test.go` file in
each package is the package name itself.
2. Rename `processor/tailsamplingprocessor/main_test.go` to
`package_test.go` for uniformity. `package_test.go` is the name we chose
for core (see discussion
[here](open-telemetry/opentelemetry-collector#9173 (comment))).
3. Run `make gotidy` to add `goleak` dependency.

I included the script used to make these changes [in the
issue](open-telemetry#30438 (comment)).

**Link to tracking Issue:** <Issue number if applicable>
open-telemetry#30438
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Feb 1, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
This PR contains 3 main changes:
1. Make a copy of the same file into every package whose `goleak` check
is currently passing. The only change for the `package_test.go` file in
each package is the package name itself.
2. Rename `processor/tailsamplingprocessor/main_test.go` to
`package_test.go` for uniformity. `package_test.go` is the name we chose
for core (see discussion
[here](open-telemetry/opentelemetry-collector#9173 (comment))).
3. Run `make gotidy` to add `goleak` dependency.

I included the script used to make these changes [in the
issue](open-telemetry#30438 (comment)).

**Link to tracking Issue:** <Issue number if applicable>
open-telemetry#30438
anthoai97 pushed a commit to anthoai97/opentelemetry-collector-contrib that referenced this pull request Feb 12, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
This PR contains 3 main changes:
1. Make a copy of the same file into every package whose `goleak` check
is currently passing. The only change for the `package_test.go` file in
each package is the package name itself.
2. Rename `processor/tailsamplingprocessor/main_test.go` to
`package_test.go` for uniformity. `package_test.go` is the name we chose
for core (see discussion
[here](open-telemetry/opentelemetry-collector#9173 (comment))).
3. Run `make gotidy` to add `goleak` dependency.

I included the script used to make these changes [in the
issue](open-telemetry#30438 (comment)).

**Link to tracking Issue:** <Issue number if applicable>
open-telemetry#30438
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants