Skip to content
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

internal/apps: Add two new apps #2868

Merged
merged 7 commits into from
Sep 18, 2024
Merged

internal/apps: Add two new apps #2868

merged 7 commits into from
Sep 18, 2024

Conversation

felixge
Copy link
Member

@felixge felixge commented Sep 14, 2024

What does this PR do?

Add two new internal apps. They are not meant to run in CI (not hooked up), but rather to stage screenshots for blog posts when we need them.

Also upgrade internal apps to use go1.23 since I sometimes get a go_version: tag in my screenshot and I want it to be the latest. Additionally I wanted to use some go1.23 convenience features while coding up the test apps.

Motivation

The primary motivation is to produce screenshots for an upcoming blog post about cost savings.

One part of the blog will talk about using the CPU profile to identify cost saving opportunities:

CPU

This might reveal a high amount of CPU time in GC. The allocated memory profile can be used to figure out how to reduce the frequency of GC cycles:

Allocated Memory

Additionally the Heap Live Objects profile can help with figuring out how to reduce the cost per GC cycle.

Heap Live Objects

For apps that are bottlenecked on Memory rather than CPU, the Heap Live Size profile can help.

Heap Live Size

And for queue processing applications that don't manage to utilize their available HW resources efficiently, the timeline feature can help with revealing Off-CPU bottlenecks:

Timeline Worker Pool Bottleneck

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!

@pr-commenter
Copy link

pr-commenter bot commented Sep 14, 2024

Benchmarks

Benchmark execution time: 2024-09-17 09:41:31

Comparing candidate commit 111c73d in PR branch PROF-9130/demo-apps-for-blog with baseline commit 1f0966d in branch main.

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

@felixge felixge changed the title internal/apps: Use latest go version (1.23) internal/apps: Implement gc-overhead app Sep 14, 2024
@felixge felixge changed the title internal/apps: Implement gc-overhead app internal/apps: Add gc-overhead app Sep 14, 2024
The internal apps are usually meant to demonstrate new features or to
produce screenshots for blog posts. In both cases it is better to be
using the latest version of Go as it might contain new features that are
needed, or simply to make the screenshots show an up-to-date Go version.
This allows giving nicer names to the services for screenshots.
@felixge felixge changed the title internal/apps: Add gc-overhead app internal/apps: Add two new apps Sep 17, 2024
@felixge felixge marked this pull request as ready for review September 17, 2024 06:52
@felixge felixge requested a review from a team as a code owner September 17, 2024 06:52
internal/apps/go.mod Outdated Show resolved Hide resolved
.github/workflows/smoke-tests.yml Outdated Show resolved Hide resolved
Remove go version workaround. This will be addressed by merging
#2871 first.

Use go1.23.0 instead of go1.23 since the latter includes pre-release
versions which we don't want here.
@felixge felixge changed the base branch from main to PROF-9130/move-smoke-test September 17, 2024 08:57
@felixge felixge changed the base branch from PROF-9130/move-smoke-test to main September 17, 2024 08:57
@felixge
Copy link
Member Author

felixge commented Sep 17, 2024

Updated. But needs #2871 to land first.

@felixge felixge removed the request for review from Julio-Guerra September 17, 2024 09:28
Copy link
Contributor

@nsrip-dd nsrip-dd left a comment

Choose a reason for hiding this comment

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

LGTM!

internal/apps/gc-overhead/main.go Outdated Show resolved Hide resolved
Co-authored-by: Nick Ripley <nick.ripley@datadoghq.com>
@felixge felixge enabled auto-merge (squash) September 18, 2024 12:43
@felixge felixge merged commit 3646321 into main Sep 18, 2024
162 of 166 checks passed
@felixge felixge deleted the PROF-9130/demo-apps-for-blog branch September 18, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants