-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[connector/datadog] Use the native OTel ingest API in APM stats #33297
Conversation
go.mod
Outdated
github.com/DataDog/datadog-agent/pkg/remoteconfig/state v0.54.0-rc.5 // indirect | ||
github.com/DataDog/datadog-agent/pkg/status/health v0.54.0-rc.5 // indirect | ||
github.com/DataDog/datadog-agent/pkg/telemetry v0.54.0-rc.5 // indirect | ||
github.com/DataDog/datadog-agent/pkg/trace v0.54.0-rc.5 // indirect | ||
github.com/DataDog/datadog-agent/pkg/trace v0.55.0-devel.0.20240529144302-ebeee88c4ff5 // indirect |
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.
Using a commit version for now, probably need to wait for the official release before merging this PR.
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.
Blocking merge based on this comment (#33297 (comment))
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.
This is now unblocked with #33726
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.
Looks good overall have few nit comments
func newTraceToMetricConnectorNative(set component.TelemetrySettings, cfg component.Config, metricsConsumer consumer.Metrics, metricsClient statsd.ClientInterface) (*traceToMetricConnectorNative, error) { | ||
set.Logger.Info("Building datadog connector for traces to metrics") | ||
in := make(chan *pb.StatsPayload, 100) | ||
set.MeterProvider = noop.NewMeterProvider() // disable metrics for the connector |
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.
why are we doing it ?
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.
This is a copy of https://github.com/open-telemetry/opentelemetry-collector-contrib/blame/main/connector/datadogconnector/connector.go#L69 and was added in 9a34aea#diff-63ca951f32af68231d95ef502d34657b930abb3d1a6ffaa8db864a28f3a17d6aR51
cc @mx-psi should have more context
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.
As the code stands, if we enable this, we may have duplicate metrics from the connector and exporter with the exact same name and tags. We can add support for this, but we need to fix that first.
go.mod
Outdated
github.com/DataDog/datadog-agent/pkg/remoteconfig/state v0.54.0-rc.5 // indirect | ||
github.com/DataDog/datadog-agent/pkg/status/health v0.54.0-rc.5 // indirect | ||
github.com/DataDog/datadog-agent/pkg/telemetry v0.54.0-rc.5 // indirect | ||
github.com/DataDog/datadog-agent/pkg/trace v0.54.0-rc.5 // indirect | ||
github.com/DataDog/datadog-agent/pkg/trace v0.55.0-devel.0.20240529144302-ebeee88c4ff5 // indirect |
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.
Blocking merge based on this comment (#33297 (comment))
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@songy23 do you all have any benchmarks or details about the potential performance benefits of using native ingest API? I want to know if it may help improve the overall performance of our collector pipelines. |
@arielvalentin if your traces have lots of attributes (especially when you have high cardinality of container tags), the new API has better throughput. If your traces are fairly small (with few attributes), the throughput / performance of old and new APIs are roughly on-par. |
Description:
Add a feature gate
connector.datadogconnector.NativeIngest
that enables datadog connector to use the new native OTel API in APM stats computation. It is disabled by default.Follow-up of DataDog/datadog-agent#23503.