-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[exporter/datadog] Refactor Datadog Exporter to use logs agent exporter #32327
[exporter/datadog] Refactor Datadog Exporter to use logs agent exporter #32327
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.
We probably need to support a few logs agent configs, e.g. https://github.com/DataDog/datadog-agent/blob/main/pkg/config/config_template.yaml#L1281 & https://github.com/DataDog/datadog-agent/blob/main/pkg/config/config_template.yaml#L1244
Can be done in follow-up PRs
Config: cfgComponent, | ||
Hostname: hostnameComponent, | ||
}) | ||
err = logsAgent.Start(ctx) |
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 are starting the logsAgent during component creation time. We should not do this, that's what exporterhelper.WithStart
is used for
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.
logsAgent.Start
also sets up the pipeline channel that gets passed into the logsAgentExporter, and logsAgentExporter.ConsumeLogs
gets passed into the exporterhelper at the end of createLogsExporter
. Is there a way to set this all up while also using exporterhelper.WithStart
? I don't think we follow this pattern elsewhere in the datadog exporter
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.
Hm, so this has come up recently on open-telemetry/opentelemetry-collector/issues/10031. It's true that there are other things doing the wrong thing in our exporter, I will file a ticket for those. If we can solve this problem now for this one though it would be great not to make the problem worse.
Config: cfgComponent, | ||
Hostname: hostnameComponent, | ||
}) | ||
err = logsAgent.Start(ctx) |
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.
Hm, so this has come up recently on open-telemetry/opentelemetry-collector/issues/10031. It's true that there are other things doing the wrong thing in our exporter, I will file a ticket for those. If we can solve this problem now for this one though it would be great not to make the problem worse.
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
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.
One small comment and we are done! 🚀
Description:
This PR adds support for the logs agent exporter in order to export logs to Datadog. The following changes were made in order to support this:
peer_service_aggregation
config option which was deprecated and removed since last datadog-agent version: apm: rm deprecated peer_service_aggregation config DataDog/datadog-agent#23284Link to tracking Issue:
Fixes #30099
Testing:
Documentation: