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

Jump to bottom

[chore][connector/datadogconnector] Fix connector traceToMetrics lifecycle #31811

Merged
merged 7 commits into from Mar 19, 2024
Merged

[chore][connector/datadogconnector] Fix connector traceToMetrics lifecycle #31811

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1760a6c
[chore][connector/datadogconnector] Fix connector traceToMetrics life…
mackjmr Mar 18, 2024
25a2972
run make gotidy
mackjmr Mar 18, 2024
77abc41
Update connector/datadogconnector/metadata.yaml
mackjmr Mar 18, 2024
23f34cf
Merge branch 'main' of github.com:open-telemetry/opentelemetry-collec…
mackjmr Mar 18, 2024
f053740
Merge branch 'main' into fix-datadogconnector-lifecycle
mackjmr Mar 18, 2024
8e3f9a4
Merge branch 'fix-datadogconnector-lifecycle' of github.com:mackjmr/o…
mackjmr Mar 18, 2024
cf7ba4e
remove config
mackjmr Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions connector/datadogconnector/connector.go
View file
Open in desktop
Expand Up @@ -42,6 +42,9 @@ type traceToMetricConnector struct {

// exit specifies the exit channel, which will be closed upon shutdown.
exit chan struct{}

// isStarted tracks whether Start() has been called.
isStarted bool
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an atomic? I am not sure what are the expectations regarding thread safety 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

This gets written to in Start and Read in Shutdown, so I guess the question is whether these two functions can be called concurrently or if Start can get called concurrently.

The docs for Shutdown mention:

	// This method must be safe to call:
	//   - without Start() having been called
	//   - if the component is in a shutdown state already
        ...
	Shutdown(ctx context.Context) error

It doesn't mention that Shutdown should be safe to call concurrently with Start.

For Start, the documentation doesn't mention anything about concurrency either.

So I think this should be fine ? It may be good to get this clarified in the Component docs.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, we can handle that separately. Merging...

}

var _ component.Component = (*traceToMetricConnector)(nil) // testing that the connectorImp properly implements the type Component interface
Expand Down Expand Up @@ -91,11 +94,17 @@ func (c *traceToMetricConnector) Start(_ context.Context, _ component.Host) erro
c.logger.Info("Starting datadogconnector")
c.agent.Start()
go c.run()
c.isStarted = true
return nil
}

// Shutdown implements the component.Component interface.
func (c *traceToMetricConnector) Shutdown(context.Context) error {
if !c.isStarted {
// Note: it is not necessary to manually close c.exit, c.in and c.agent.(*datadog.TraceAgent).exit channels as these are unused.
c.logger.Info("Requested shutdown, but not started, ignoring.")
return nil
}
c.logger.Info("Shutting down datadog connector")
c.logger.Info("Stopping datadog agent")
// stop the agent and wait for the run loop to exit
Expand Down
68 changes: 68 additions & 0 deletions connector/datadogconnector/generated_component_test.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion connector/datadogconnector/go.mod
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@ require (
github.com/open-telemetry/opentelemetry-collector-contrib/processor/tailsamplingprocessor v0.96.0
github.com/stretchr/testify v1.9.0
go.opentelemetry.io/collector/component v0.96.1-0.20240306115632-b2693620eff6
go.opentelemetry.io/collector/confmap v0.96.1-0.20240306115632-b2693620eff6
go.opentelemetry.io/collector/connector v0.96.1-0.20240306115632-b2693620eff6
go.opentelemetry.io/collector/consumer v0.96.1-0.20240306115632-b2693620eff6
go.opentelemetry.io/collector/exporter v0.96.1-0.20240306115632-b2693620eff6
Expand Down Expand Up @@ -148,7 +149,6 @@ require (
go.opentelemetry.io/collector/config/configtelemetry v0.96.1-0.20240306115632-b2693620eff6 // indirect
go.opentelemetry.io/collector/config/configtls v0.96.1-0.20240306115632-b2693620eff6 // indirect
go.opentelemetry.io/collector/config/internal v0.96.1-0.20240306115632-b2693620eff6 // indirect
go.opentelemetry.io/collector/confmap v0.96.1-0.20240306115632-b2693620eff6 // indirect
go.opentelemetry.io/collector/confmap/converter/expandconverter v0.96.1-0.20240306115632-b2693620eff6 // indirect
go.opentelemetry.io/collector/confmap/provider/envprovider v0.96.1-0.20240306115632-b2693620eff6 // indirect
go.opentelemetry.io/collector/confmap/provider/fileprovider v0.96.1-0.20240306115632-b2693620eff6 // indirect
Expand Down
4 changes: 2 additions & 2 deletions connector/datadogconnector/metadata.yaml
View file
Open in desktop
Expand Up @@ -11,5 +11,5 @@ status:
emeritus: [gbbr]

tests:
skip_lifecycle: true
skip_shutdown: true
config:
mackjmr marked this conversation as resolved.
Show resolved Hide resolved