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] Use statswriter, client aggregator is for tracer stats only #31173
[exporter/datadog] Use statswriter, client aggregator is for tracer stats only #31173
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.
Can you add a changelog? https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/.chloggen/TEMPLATE.yaml
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.
✅ the fundamental change looks right to me
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.
The statswriter code isn't setting the tracer version as done for clientstatsAggregator(
statsv := exp.params.BuildInfo.Command + exp.params.BuildInfo.Version |
It looks like tracer version is just an "informative" field, I ran it locally and did verify that the new code gave an empty string. So to keep the previous behavior I just added a commit that will attach the tracer version to the client stats payload if it's not already there |
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.
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 great.
…tats only (open-telemetry#31173) **Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> The `ProcessStats` function receives "tracer stats" payloads, by looping through the payload and breaking it apart this drastically increases the number of stats payloads being emitted. Instead, let's write directly to the StatsWriter, which reduces duplicated work and keeps the payload together as a single payload. **Link to tracking Issue:** <Issue number if applicable> **Testing:** <Describe what testing was performed and which tests were added.> I tested this locally with the otel example calendar app verifying stats were calculated correctly as well, the logs also showed the reduction in the number of payloads being emitted. ~TODO: I will add a test to prevent future regression~ Done! **Documentation:** <Describe the documentation added.> --------- Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
…tats only (open-telemetry#31173) **Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> The `ProcessStats` function receives "tracer stats" payloads, by looping through the payload and breaking it apart this drastically increases the number of stats payloads being emitted. Instead, let's write directly to the StatsWriter, which reduces duplicated work and keeps the payload together as a single payload. **Link to tracking Issue:** <Issue number if applicable> **Testing:** <Describe what testing was performed and which tests were added.> I tested this locally with the otel example calendar app verifying stats were calculated correctly as well, the logs also showed the reduction in the number of payloads being emitted. ~TODO: I will add a test to prevent future regression~ Done! **Documentation:** <Describe the documentation added.> --------- Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Description:
The
ProcessStats
function receives "tracer stats" payloads, by looping through the payload and breaking it apart this drastically increases the number of stats payloads being emitted. Instead, let's write directly to the StatsWriter, which reduces duplicated work and keeps the payload together as a single payload.Link to tracking Issue:
Testing:
I tested this locally with the otel example calendar app verifying stats were calculated correctly as well, the logs also showed the reduction in the number of payloads being emitted.
TODO: I will add a test to prevent future regressionDone!Documentation: