Skip to content

Regenerate stats payload to add is_trace_root#2856

Merged
mtoffl01 merged 2 commits into
mainfrom
andrew.glaude/statsMsgp
Sep 16, 2024
Merged

Regenerate stats payload to add is_trace_root#2856
mtoffl01 merged 2 commits into
mainfrom
andrew.glaude/statsMsgp

Conversation

@ajgajg1134

@ajgajg1134 ajgajg1134 commented Sep 11, 2024

Copy link
Copy Markdown
Contributor

What does this PR do?

Regenerate the stats payload msgp encoder as it was missing the new is_trace_root field.

Motivation

We want is_trace_root to be available for these metrics. Eventually this code will go away with my PR here #2817, but until that PR is ready after an agent review (still a few weeks out at least) then this will resolve the problem before then.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@ajgajg1134
ajgajg1134 requested a review from a team as a code owner September 11, 2024 17:49
@pr-commenter

pr-commenter Bot commented Sep 11, 2024

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2024-09-11 18:14:56

Comparing candidate commit d893820 in PR branch andrew.glaude/statsMsgp with baseline commit d1b31e8 in branch main.

Found 1 performance improvements and 7 performance regressions! Performance is the same for 50 metrics, 1 unstable metrics.

scenario:BenchmarkPartialFlushing/Disabled-24

  • 🟥 execution_time [+10.209ms; +13.073ms] or [+3.779%; +4.840%]

scenario:BenchmarkPartialFlushing/Enabled-24

  • 🟥 execution_time [+11.147ms; +14.022ms] or [+4.067%; +5.116%]

scenario:BenchmarkSetTagString-24

  • 🟩 execution_time [-9.094ns; -6.426ns] or [-7.618%; -5.383%]

scenario:BenchmarkSingleSpanRetention/no-rules-24

  • 🟥 execution_time [+9.045µs; +10.213µs] or [+3.772%; +4.259%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24

  • 🟥 execution_time [+9.149µs; +10.775µs] or [+3.779%; +4.450%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24

  • 🟥 execution_time [+9.482µs; +10.711µs] or [+3.924%; +4.432%]

scenario:BenchmarkStartSpan-24

  • 🟥 execution_time [+93.007ns; +112.393ns] or [+4.126%; +4.986%]

scenario:BenchmarkTracerAddSpans-24

  • 🟥 execution_time [+136.149ns; +173.451ns] or [+3.495%; +4.453%]

@darccio

darccio commented Sep 12, 2024

Copy link
Copy Markdown
Member

@ajgajg1134 I see some unrelated generated changes and I'm worried if they were missing for a long time ago or not.

@ajgajg1134

ajgajg1134 commented Sep 12, 2024

Copy link
Copy Markdown
Contributor Author

@ajgajg1134 I see some unrelated generated changes and I'm worried if they were missing for a long time ago or not.

Yeah it appears that in addition to the new field there's also more specific error messages when a field fails to serialize. This seems fine to include here as well, do you see any other changes I'm missing?

knusbaum
knusbaum approved these changes Sep 12, 2024
View reviewed changes

@knusbaum knusbaum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this.

@darccio

darccio commented Sep 12, 2024

Copy link
Copy Markdown
Member

do you see any other changes I'm missing?

Not really, but I was surprised by all the changes. In the generator we trust.

@mtoffl01
mtoffl01 merged commit d9505b3 into main Sep 16, 2024
@mtoffl01
mtoffl01 deleted the andrew.glaude/statsMsgp branch September 16, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@darccio darccio darccio approved these changes
@knusbaum knusbaum knusbaum approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ajgajg1134 @darccio @knusbaum @mtoffl01