-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[*/datadog] Move datadogconnector functionality into pkg/datadog #43636
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
[*/datadog] Move datadogconnector functionality into pkg/datadog #43636
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From high level I feel you moved too many to pkg/datadog and left too few in connector/datadog (only the factory.go file?), which doesn't seem idiomatic. WDYT @mx-psi
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package datadogconnector // import "github.com/open-telemetry/opentelemetry-collector-contrib/connector/datadogconnector" | ||
| package apmstats // import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/datadog/apmstats" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this file need to be moved? It's just a benchmark test
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package datadogconnector // import "github.com/open-telemetry/opentelemetry-collector-contrib/connector/datadogconnector" | ||
| package apmstats // import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/datadog/apmstats" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file can stay in connector since most code is already in pkg/datadog/config
| } | ||
|
|
||
| func getTraceAgentCfg(logger *zap.Logger, cfg datadogconfig.TracesConnectorConfig, attributesTranslator *attributes.Translator) *config.AgentConfig { | ||
| func getTraceAgentCfg(logger *zap.Logger, cfg datadogconfig.TracesConnectorConfig, attributesTranslator *attributes.Translator, tagger types.TaggerClient, hostnameOpt option.Option[string]) *config.AgentConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from this function, can the rest of this file stay in the connector package?
| // Replace old monolithic genproto with newer version to avoid ambiguous import with split modules | ||
| replace google.golang.org/genproto => google.golang.org/genproto v0.0.0-20231030173426-d783a09b4405 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not do this, this only pins the dependency on this module but not on any modules that import this one, which means any customer using Datadog components on a custom build may have to add this replace to their Collector builder configuraiton
From https://go.dev/ref/mod#go-mod-file-replace:
replace directives only apply in the main module’s go.mod file and are ignored in other modules
I agree, I think we should move as little as possible |
|
Changed the approach based on comments, closing and replacing with #43755 |
Description
The majority of logic between the datadog-agent fork of datadogconnector and the copy in this repo is shared. Factor out that shared logic into pkg/datadog so that it can be imported in datadog-agent.
Pin google.golang.org/genproto version to avoid ambiguous import issue that caused conflict between different versions of the dependency pulled in by different components