[chore][exporter/datadog] Only start hostmetadata.Reporter if host_metadata.enabled
#36669
Conversation
github-actions
bot
requested review from
ankitpatel96,
dineshg13,
liustanley,
mackjmr,
mx-psi and
songy23
jade-guiton-dd
changed the title
hostmetadata.Reporter if host_metadata.enabledhostmetadata.Reporter if host_metadata.enabled
exporter/datadogexporter/factory.go
Outdated
| cancel() | ||
| return nil, fmt.Errorf("failed to build host metadata reporter: %w", err) | ||
|
|
||
| // These variables are referenced below, but only used if HostMetadata.Enabled |
There was a problem hiding this comment.
I find this comment a bit confusing, why not define them inside the if block then?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
I simplified the logic and the comments somewhat, tell me if you think things are clearer or not
…_metadata.enabled` (open-telemetry#36669) #### Description This PR avoids creating an `inframetadata.Reporter` and starting the metadata-sending goroutine when `host_metadata.enabled` is `false`, and removes some code that was used to work around the previous behavior. See tracking issue for context. When `enabled` is `false`, we leave the `exporter.metadataReporter` field as `nil`. The parts of the code accessing this field were already gated behind `enabled: true`, or `only_metadata: true` (which, in a valid config, implies `enabled: false`), so it should not cause issues. However, tests that modify the config after the exporter is created, or which manually create invalid `Config` structs may encounter segfaults. I assumed we do not mind segfaults in cases of incorrect internal use, and fixed the one test that was failing because of this. #### Link to tracking issue Fixes open-telemetry#36522
…_metadata.enabled` (open-telemetry#36669) #### Description This PR avoids creating an `inframetadata.Reporter` and starting the metadata-sending goroutine when `host_metadata.enabled` is `false`, and removes some code that was used to work around the previous behavior. See tracking issue for context. When `enabled` is `false`, we leave the `exporter.metadataReporter` field as `nil`. The parts of the code accessing this field were already gated behind `enabled: true`, or `only_metadata: true` (which, in a valid config, implies `enabled: false`), so it should not cause issues. However, tests that modify the config after the exporter is created, or which manually create invalid `Config` structs may encounter segfaults. I assumed we do not mind segfaults in cases of incorrect internal use, and fixed the one test that was failing because of this. #### Link to tracking issue Fixes open-telemetry#36522
Description
This PR avoids creating an
inframetadata.Reporterand starting the metadata-sending goroutine whenhost_metadata.enabledisfalse, and removes some code that was used to work around the previous behavior. See tracking issue for context.When
enabledisfalse, we leave theexporter.metadataReporterfield asnil. The parts of the code accessing this field were already gated behindenabled: true, oronly_metadata: true(which, in a valid config, impliesenabled: false), so it should not cause issues. However, tests that modify the config after the exporter is created, or which manually create invalidConfigstructs may encounter segfaults. I assumed we do not mind segfaults in cases of incorrect internal use, and fixed the one test that was failing because of this.Link to tracking issue
Fixes #36522