Skip to content

Commit b228861

Browse files
zimegClaude
andcommitted
fix: avoid hang before command exit of jaeger paths
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
1 parent 1059ec2 commit b228861

4 files changed

Lines changed: 29 additions & 9 deletions

File tree

internal/config/dotenv.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,18 @@ import (
1919

2020
"github.com/joho/godotenv"
2121
"github.com/slackapi/slack-cli/internal/pkg/version"
22+
"github.com/slackapi/slack-cli/internal/shared/types"
2223
"github.com/spf13/afero"
2324
)
2425

26+
// IsTelemetryDisabled checks environment variables to determine if telemetry
27+
// should be disabled. This can be called before Config is initialized.
28+
func IsTelemetryDisabled(os types.Os) bool {
29+
var disableTelemetry = strings.TrimSpace(os.Getenv(slackDisableTelemetryEnv))
30+
var testVersion = strings.TrimSpace(os.Getenv(version.EnvTestVersion))
31+
return (disableTelemetry != "" && disableTelemetry != "false" && disableTelemetry != "0") || testVersion != ""
32+
}
33+
2534
// GetDotEnvFileVariables collects only the variables in the .env file
2635
func (c *Config) GetDotEnvFileVariables() (map[string]string, error) {
2736
variables := map[string]string{}
@@ -60,9 +69,7 @@ func (c *Config) LoadEnvironmentVariables() error {
6069
}
6170

6271
// Disable telemetry if either disable-telemetry or test-version environment variables
63-
var disableTelemetry = strings.TrimSpace(c.os.Getenv(slackDisableTelemetryEnv))
64-
var testVersion = strings.TrimSpace(c.os.Getenv(version.EnvTestVersion))
65-
if (disableTelemetry != "" && disableTelemetry != "false" && disableTelemetry != "0") || testVersion != "" {
72+
if IsTelemetryDisabled(c.os) {
6673
c.DisableTelemetryFlag = true
6774
}
6875

internal/slackcontext/slackcontext_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ func Test_SlackContext_SetOpenTracingTraceID(t *testing.T) {
112112
}
113113

114114
func Test_SlackContext_OpenTracingTracer(t *testing.T) {
115-
_, _tracer := tracer.SetupTracer(true)
115+
_, _tracer := tracer.SetupTracer(true, false)
116116

117117
tests := map[string]struct {
118118
expectedTracer opentracing.Tracer
@@ -139,7 +139,7 @@ func Test_SlackContext_OpenTracingTracer(t *testing.T) {
139139
}
140140

141141
func Test_SlackContext_SetOpenTracingTracer(t *testing.T) {
142-
_, _tracer := tracer.SetupTracer(true)
142+
_, _tracer := tracer.SetupTracer(true, false)
143143

144144
tests := map[string]struct {
145145
expectedTracer opentracing.Tracer

internal/tracer/tracer.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424
)
2525

2626
// SetupTracer sets up the tracer to send data to honeycomb
27-
func SetupTracer(isDev bool) (io.Closer, opentracing.Tracer) {
27+
func SetupTracer(isDev bool, disableTelemetry bool) (io.Closer, opentracing.Tracer) {
2828
var collectorEndpoint = "https://slackb.com/traces/v1/jaeger"
2929
if isDev {
3030
collectorEndpoint = "https://dev.slackb.com/traces/v1/jaeger"
@@ -33,7 +33,7 @@ func SetupTracer(isDev bool) (io.Closer, opentracing.Tracer) {
3333
// Recommended configuration for production.
3434
var jCfg = jaegercfg.Configuration{
3535
ServiceName: "slack-cli", // Don't change this. Required to distinguish logs & traces coming from the CLI
36-
Disabled: false,
36+
Disabled: disableTelemetry,
3737
Reporter: &jaegercfg.ReporterConfig{
3838
LogSpans: false,
3939
CollectorEndpoint: collectorEndpoint,

main.go

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,14 @@ import (
1919
"os"
2020
"runtime/debug"
2121
"strings"
22+
"time"
2223

2324
"github.com/google/uuid"
2425
"github.com/opentracing/opentracing-go"
2526
"github.com/slackapi/slack-cli/cmd"
27+
"github.com/slackapi/slack-cli/internal/config"
2628
"github.com/slackapi/slack-cli/internal/goutils"
29+
"github.com/slackapi/slack-cli/internal/slackdeps"
2730
"github.com/slackapi/slack-cli/internal/iostreams"
2831
"github.com/slackapi/slack-cli/internal/ioutils"
2932
"github.com/slackapi/slack-cli/internal/pkg/version"
@@ -43,8 +46,18 @@ func main() {
4346
// - This would allow us to choose the correct API host based on flags
4447
// - Uncomment `isDevTarget` if we refactor to cmd/root.go and update to call `ResolveAPIHost`
4548
// var isDevTarget = shared.NewClientFactory().AuthClient().UserDefaultAuthIsProd(ctx) // TODO - hack, remove shared.clients
46-
var jaegerCloser, tracer = tracer.SetupTracer(false) // Always setup open tracing on prod
47-
defer jaegerCloser.Close()
49+
var jaegerCloser, tracer = tracer.SetupTracer(false, config.IsTelemetryDisabled(slackdeps.NewOs()))
50+
defer func() {
51+
done := make(chan struct{})
52+
go func() {
53+
jaegerCloser.Close()
54+
close(done)
55+
}()
56+
select {
57+
case <-done:
58+
case <-time.After(2 * time.Second):
59+
}
60+
}()
4861
ctx = slackcontext.SetOpenTracingTracer(ctx, tracer)
4962

5063
// Set context values

0 commit comments

Comments
 (0)