Overhead testing foundations #3627
Conversation
testing-overhead/src/test/java/io/opentelemetry/LatestAgentSnapshotResolver.java
Outdated
Show resolved
Hide resolved
testing-overhead/src/test/java/io/opentelemetry/PetClinicRestContainer.java
Outdated
Show resolved
Hide resolved
breedx-splk
force-pushed
the
overhead_testing_foundations
branch
from
82095f9 to
148a526
Compare
|
|
||
| private final static int DEFAULT_MAX_REQUEST_RATE = 0; // none | ||
| private final static int DEFAULT_CONCURRENT_CONNECTIONS = 5; | ||
| private final static int DEFAULT_TOTAL_ITERATIONS = 500; |
There was a problem hiding this comment.
500 iterations seems too little. Ideally there should be a warmup iteration before measurement iteration so that we would measure application in steady state where it has already jit compiled everything it can.
There was a problem hiding this comment.
That's reasonable -- I think I agree that 500 is too little and we should probably bump it up once things settle down and we're actually persisting data. For development, 500 is nice because you don't have to wait too long.
As far as warmup -- I'm torn, because if the agent impacts startup and jit (with more code, the jitter will probably have to work harder, right?) -- maybe we do want to account for that? Does allowing the app to reach steady state seem slightly unfair? Maybe we're also talking about two different types of tests, so I could see this maybe becoming a parameter of the configuration. 🤔
There was a problem hiding this comment.
Maybe we're also talking about two different types of tests
👍
There was a problem hiding this comment.
Our users are likely to care less about startup overhead than steady state performance.
There was a problem hiding this comment.
Ok. @laurit would you be ok with deferring a warmup enhancement for a future PR?
breedx-splk
requested review from
anuraaga,
jkwatson,
mateuszrzeszutek,
pavolloffay and
tylerbenson
as code owners
| @@ -0,0 +1,22 @@ | |||
| plugins { | |||
| id("java") | |||
There was a problem hiding this comment.
Can we avoid the yet another gradle wrapper and use otel.java-conventions? Any particular reason for the separation?
There was a problem hiding this comment.
Would that imply adding this subdir as a submodule of the main project?
My desire for keeping this stand-alone is primarily motivated by the massive size of the base project and the fact that it commonly takes 9000 years to open in IDEA and index. It's MUCH easier/simpler/faster to work in this project when the rest of the instrumentation is not in scope.
I also understand/disgust in having so many gradle wrappers in the repo...but is it really worse than having everything in one MASSIVE gradle project? I dunno. Curious what others think.
There was a problem hiding this comment.
If adding one gradle dependency to this separate project requires re-importing of all our 100s of instrumentation - that is much larger overkill to me than yet another gradle wrapper.
Also, don't forget that you don't actually need a separate wrapper for every independent project. ../gradlwe build works just fine.
| if(Agent.NONE.equals(agent)){ | ||
| return Optional.empty(); | ||
| } | ||
| if(Agent.LATEST_SNAPSHOT.equals(agent)){ |
There was a problem hiding this comment.
Instead of snapshot, which includes the annoying resolver, how about just building from this repo and using that agent, similar to our smoke tests? The snapshot can get old due to main build failures such as testLatestDeps so that would be easier to reason about which agent commit is actually teste
There was a problem hiding this comment.
I had really hoped to avoid this and to keep this test process decoupled from the main build. It's going to slow things down considerably.
Is the thought that regressions would be harder to track back to commits or something?
There was a problem hiding this comment.
I support Jason in his reluctance to build local project every time.
There was a problem hiding this comment.
WDYT about renaming the module to benchmark-overhead? Usually our testing... modules are for correctness testing, we could keep all performance testing in benchmark... modules (also see #3672 (comment))
testing-overhead/src/test/java/io/opentelemetry/agents/AgentResolver.java
Outdated
Show resolved
Hide resolved
testing-overhead/src/test/java/io/opentelemetry/agents/AgentResolver.java
Outdated
Show resolved
Hide resolved
testing-overhead/src/test/java/io/opentelemetry/agents/LatestAgentSnapshotResolver.java
Outdated
Show resolved
Hide resolved
testing-overhead/src/test/java/io/opentelemetry/agents/AgentResolver.java
Outdated
Show resolved
Hide resolved
I'm fine wit that changes, I wasn't aware of the subtle semantic difference...so that's cooll. However I do have a pile of commits stacked up in #3648 and I'm skeptical of my abilities to cleanly rebase/pick them after a name change. My preference would be to do the rename in a future PR. |
sounds good 👍 |
There was a problem hiding this comment.
I am OK with this as POC to get MVP running. We then can iterate.
👍
@breedx-splk can you open an issue to (briefly) summarize open questions from above? then let's merge
|
Thanks everyone. I captured the open things as 3 issues and assigned them to me for follow-up. |
This should be blocked by #3554 and #3640.Good to review now.It sets up the basic project which uses testcontainers and a set of test configurations to determine which test runs to perform. It stands up a collector, then a petclinic container with the given agent (which might be
none) and then runs k6 against the running app. It repeats this for each configuration.While I think the configuration stuff is pretty flexible, it might be a little overengineered and I'm looking for feedback. One of the goals is to eventually support direct http/file URLs to agents (vendors might benefit from this).
The results processing bits are intended to come in a follow-up PR (there's a TODO). Right now, the results are just output to the workspace in the form of k6 json output and jfr files.