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.

Sign up for GitHub

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][exporter/datadog] Only start hostmetadata.Reporter if host_metadata.enabled #36669

Merged
merged 8 commits into from
Dec 10, 2024
Merged

[chore][exporter/datadog] Only start hostmetadata.Reporter if host_metadata.enabled #36669

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1262a34
Only start `Reporter` if `HostMetadata.Enabled`
jade-guiton-dd Dec 4, 2024
a03a583
Shadowing is bad, mkay?
jade-guiton-dd Dec 4, 2024
d587277
Fix test that was modifying config in-flight
jade-guiton-dd Dec 4, 2024
035cf82
Add `ReporterPeriod` to test config
jade-guiton-dd Dec 4, 2024
33e1fb1
Lint
jade-guiton-dd Dec 4, 2024
d3aeb98
We've fixed one, yes. But what about second broken test?
jade-guiton-dd Dec 4, 2024
46161c1
Refactor and add comments
jade-guiton-dd Dec 6, 2024
639ce64
Minify diff
jade-guiton-dd Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 37 additions & 15 deletions exporter/datadogexporter/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,11 +269,19 @@ func (f *factory) createMetricsExporter(
statsIn := make(chan []byte, 1000)
statsv := set.BuildInfo.Command + set.BuildInfo.Version
f.consumeStatsPayload(ctx, &wg, statsIn, statsWriter, statsv, acfg.AgentVersion, set.Logger)
pcfg := newMetadataConfigfromConfig(cfg)
metadataReporter, err := f.Reporter(set, pcfg)
if err != nil {
cancel()
return nil, fmt.Errorf("failed to build host metadata reporter: %w", err)

// These variables are referenced below, but only used if HostMetadata.Enabled
Copy link
Member

Choose a reason for hiding this comment

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

I find this comment a bit confusing, why not define them inside the if block then?

Copy link
Contributor Author

@jade-guiton-dd jade-guiton-dd Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Because they're referenced below. Unless we structure the whole function, we need these variables to be defined for the rest of the function, with metadataReporter left as nil if the if branch wasn't taken.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I find the usage of "referenced" and "used" here confusing on the comment then: from your message I take that they are "used" (mentioned in code) both if HostMetadata.Enabled and if not, right?

Copy link
Contributor Author

@jade-guiton-dd jade-guiton-dd Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right, I guess we have different definitions of "used".

The two variables are mentioned / referenced in the code below, which requires their scope to be larger than the if statement if we want the code to compile. This is independent of what path the code takes.

Even when !HostMetadata.Enabled, metadataReporter will still be read (not pcfg however), in order to be passed to the new(Logs|Traces|Metrics)Exporter function. It will be nil in that case, and gets stored as the exp.metadataReporter field.

But the pointer won't be used later down the line, ie. no methods will be called on it. (Which is good, because that would cause a segfault.)

I can reword the comment to make this clearer if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, thanks! I will let you decide whether to reword it. I did find it confusing, and I don't feel like this is standard wording for this, but maybe it's just me 😄

Copy link
Contributor Author

@jade-guiton-dd jade-guiton-dd Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I simplified the logic and the comments somewhat, tell me if you think things are clearer or not

var (
pcfg hostmetadata.PusherConfig
metadataReporter *inframetadata.Reporter
)
if cfg.HostMetadata.Enabled {
pcfg = newMetadataConfigfromConfig(cfg)
metadataReporter, err = f.Reporter(set, pcfg)
if err != nil {
cancel()
return nil, fmt.Errorf("failed to build host metadata reporter: %w", err)
}
}

if cfg.OnlyMetadata {
Expand Down Expand Up @@ -374,11 +382,18 @@ func (f *factory) createTracesExporter(
return nil, fmt.Errorf("failed to start trace-agent: %w", err)
}

pcfg := newMetadataConfigfromConfig(cfg)
metadataReporter, err := f.Reporter(set, pcfg)
if err != nil {
cancel()
return nil, fmt.Errorf("failed to build host metadata reporter: %w", err)
// These variables are referenced below, but only used if HostMetadata.Enabled
var (
pcfg hostmetadata.PusherConfig
metadataReporter *inframetadata.Reporter
)
if cfg.HostMetadata.Enabled {
pcfg = newMetadataConfigfromConfig(cfg)
metadataReporter, err = f.Reporter(set, pcfg)
if err != nil {
cancel()
return nil, fmt.Errorf("failed to build host metadata reporter: %w", err)
}
}

if cfg.OnlyMetadata {
Expand Down Expand Up @@ -462,11 +477,18 @@ func (f *factory) createLogsExporter(
ctx, cancel := context.WithCancel(ctx)
// cancel() runs on shutdown

pcfg := newMetadataConfigfromConfig(cfg)
metadataReporter, err := f.Reporter(set, pcfg)
if err != nil {
cancel()
return nil, fmt.Errorf("failed to build host metadata reporter: %w", err)
// These variables are referenced below, but only used if HostMetadata.Enabled
var (
pcfg hostmetadata.PusherConfig
metadataReporter *inframetadata.Reporter
)
if cfg.HostMetadata.Enabled {
pcfg = newMetadataConfigfromConfig(cfg)
metadataReporter, err = f.Reporter(set, pcfg)
if err != nil {
cancel()
return nil, fmt.Errorf("failed to build host metadata reporter: %w", err)
}
}

attributesTranslator, err := f.AttributesTranslator(set.TelemetrySettings)
Expand Down
6 changes: 0 additions & 6 deletions exporter/datadogexporter/logs_exporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,6 @@ func TestLogsExporter(t *testing.T) {
Endpoint: server.URL,
},
},
HostMetadata: HostMetadataConfig{
ReporterPeriod: 30 * time.Minute,
},
}

params := exportertest.NewNopSettings()
Expand Down Expand Up @@ -598,9 +595,6 @@ func TestLogsAgentExporter(t *testing.T) {
CompressionLevel: 6,
BatchWait: 1,
},
HostMetadata: HostMetadataConfig{
ReporterPeriod: 30 * time.Minute,
},
}
params := exportertest.NewNopSettings()
f := NewFactory()
Expand Down
15 changes: 9 additions & 6 deletions exporter/datadogexporter/metrics_exporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ import (
conventions127 "go.opentelemetry.io/collector/semconv/v1.27.0"
conventions "go.opentelemetry.io/collector/semconv/v1.6.1"
"go.uber.org/zap"

"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/datadog/config"
)

func TestNewExporter(t *testing.T) {
Expand Down Expand Up @@ -58,11 +60,14 @@ func TestNewExporter(t *testing.T) {
CumulativeMonotonicMode: CumulativeMonotonicSumModeToDelta,
},
},
HostMetadata: HostMetadataConfig{
HostMetadata: config.HostMetadataConfig{
Enabled: true,
ReporterPeriod: 30 * time.Minute,
HostnameSource: HostnameSourceFirstResource,
},
}
cfg.HostMetadata.SetSourceTimeout(50 * time.Millisecond)

params := exportertest.NewNopSettings()
f := NewFactory()

Expand All @@ -76,8 +81,6 @@ func TestNewExporter(t *testing.T) {
require.NoError(t, err)
assert.Empty(t, server.MetadataChan)

cfg.HostMetadata.Enabled = true
cfg.HostMetadata.HostnameSource = HostnameSourceFirstResource
testMetrics = pmetric.NewMetrics()
testutil.TestMetrics.CopyTo(testMetrics)
err = exp.ConsumeMetrics(context.Background(), testMetrics)
Expand Down Expand Up @@ -439,8 +442,10 @@ func TestNewExporter_Zorkian(t *testing.T) {
CumulativeMonotonicMode: CumulativeMonotonicSumModeToDelta,
},
},
HostMetadata: HostMetadataConfig{
HostMetadata: config.HostMetadataConfig{
Enabled: true,
ReporterPeriod: 30 * time.Minute,
HostnameSource: HostnameSourceFirstResource,
},
}
params := exportertest.NewNopSettings()
Expand All @@ -456,8 +461,6 @@ func TestNewExporter_Zorkian(t *testing.T) {
require.NoError(t, err)
assert.Empty(t, server.MetadataChan)

cfg.HostMetadata.Enabled = true
cfg.HostMetadata.HostnameSource = HostnameSourceFirstResource
testMetrics = pmetric.NewMetrics()
testutil.TestMetrics.CopyTo(testMetrics)
err = exp.ConsumeMetrics(context.Background(), testMetrics)
Expand Down
15 changes: 1 addition & 14 deletions exporter/datadogexporter/traces_exporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,6 @@ func TestTracesSource(t *testing.T) {
IgnoreResources: []string{},
},
},
HostMetadata: HostMetadataConfig{
ReporterPeriod: 30 * time.Minute,
},
}

assert := assert.New(t)
Expand Down Expand Up @@ -270,9 +267,6 @@ func TestTraceExporter(t *testing.T) {
},
TraceBuffer: 2,
},
HostMetadata: HostMetadataConfig{
ReporterPeriod: 30 * time.Minute,
},
}
cfg.Traces.SetFlushInterval(0.1)

Expand All @@ -298,11 +292,7 @@ func TestNewTracesExporter(t *testing.T) {
metricsServer := testutil.DatadogServerMock()
defer metricsServer.Close()

cfg := &Config{
HostMetadata: HostMetadataConfig{
ReporterPeriod: 30 * time.Minute,
},
}
cfg := &Config{}
cfg.API.Key = "ddog_32_characters_long_api_key1"
cfg.Metrics.TCPAddrConfig.Endpoint = metricsServer.URL
params := exportertest.NewNopSettings()
Expand Down Expand Up @@ -368,9 +358,6 @@ func TestPushTraceData_NewEnvConvention(t *testing.T) {
Traces: TracesConfig{
TCPAddrConfig: confignet.TCPAddrConfig{Endpoint: server.URL},
},
HostMetadata: HostMetadataConfig{
ReporterPeriod: 30 * time.Minute,
},
}
cfg.Traces.SetFlushInterval(0.1)

Expand Down
Loading