Problem/Motivation
Now that #3352459: Add OpenTelemetry Application Performance Monitoring to core performance tests is in core, and there is a public OpenTelemetry/Grafana install at http://grafana.tag1demo.tag1.io/d/teMVIdjVz/umami?orgId=1&refresh=30s we'll be able to run just the OpenTelemetry group tests with that endpoint configured to feed traces into it - giving us a performance dashboard that's updated every hour.
Steps to reproduce
See data appear on http://grafana.tag1demo.tag1.io/d/teMVIdjVz/umami?orgId=1&refresh=30s
This is fed by a pipeline schedule on a Drupal core fork: https://www.drupal.org/project/core_performance_testbed
Proposed resolution
1. Add a DAILY_TESTvariable to the daily schedule, and change the daily jobs to run only when that is set (done on gitlab courtesy of @longwave).
2. Add a new 'Performance tests' job that only runs when PERFORMANCE_TEST is set - this will be set on a different hourly pipeline schedule so we get regular and predictable data fed into grafana.
3. Couple of small bugfixes to PerformanceTestTrait - use getenv() instead of $_ENV and make sure we look for firstContentfulPaint before we stop collecting log entries.
If we notice wide variations in the AWS spot instances, we might want to move the test runs to slightly more dedicated hosting somehow, but would be good to see everything working end-to-end asap and then adjust from there. Pretty sure this could be done by adding a dedicated/hard-coded runner to gitlab and then always sending performance tests to that, although will require infra support.
The reason to run hourly rather than on-commit is twofold:
1. An hourly run means there will be data to look at even if there hasn't been a commit in 24 hours or so.
2. Conversely, if five commits are pushed within ten minutes, then either the commits would interrupt each other, leading to incomplete test runs then one final test run, or if we let them all run, the points on the graph could overlap from different test runs completing different tests.
Hourly runs smooth both of these things out. #3379761: Add the commit hash to OpenTelemetry traces will show us which commit hash produced which trace (and hopefully which point on the graph too).
Remaining tasks
To test the MR, we have to allow manual runs and hardcode the OTEL_COLLECTOR environment variable in the YAML, when this is RTBC, or immediately following an initial commit, both of these can be removed from the job YAML and configured in the gitlab UI instead.
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3391689
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3391689-pipeline
changes, plain diff MR !4990
- 3391689-pp--run-opentelemetry
changes, plain diff MR !4987
Comments
Comment #2
catchUnpostponing, initial hosted otel/grafana stack is up. Going to try to get a working job first, then add the actual OTEL_COLLECTOR bit.
Starting with a manual job to test the actual gitlab integration, but will then need to figure out the scheduling bit - we might need separate env vars for daily vs hourly schedules for this I think which should be doable by adding custom variables in the schedule UI.
Comment #4
catchI think this is working, but lost access to the grafana instance overnight so waiting for people to wake up...
This is only so far a proof of concept to populate the graph vs. manual runs, various @todos:
1. We should run this only on one database to start with, probably MySQL 8. Later on we could add database type to traces and add it as a selector in the dashboard similar to version. So need to hard-code that somehow, possibly it should live in the parent job/gitlab-ci.yml
2. We need an hourly scheduled pipeline, that only runs this job, this means adding custom variables to the daily pipelines so that daily jobs can add checks for those in a rules: section to not also run hourly.
3. The OTEL_COLLECTOR endpoint definition can go in a custom variable in the scheduled job, and then we can look for that and export it as an environment variable.
Comment #6
catchReady for review more or less.
This should be set in the gitlab schedule, but for that to work, we need 1. a schedule and 2. for this to be committed, so a bit chicken-and-egg.
This should be moved to a follow-up, which will depend on getting the branch name from gitlab, and the otel/grafana dashboard actually being stable, but for now it's the only way to run the pipeline to test it, without a schedule being configured, so again, chicken and egg.
There is probably a better way to tie this to the default database and php combination than hard-coding the values, but I don't know what that is yet.
Comment #7
catchComment #8
catchComment #9
catchThis is working properly now, you can see the graphs and drill down into traces on http://grafana.tag1demo.tag1.io/d/teMVIdjVz/umami?orgId=1&refresh=30s&fr...
Comment #10
catchThere's a chicken and egg with this issue and core commits, because we can't validate scheduled pipeline changes until after commit since the variables aren't set on MRs.
To work around this, I created a Drupal core fork on d.o, and configured a schedule there the same way it would be configured for core.
Tests are now running on that fork for both the 11.x and 10.2.x branches every 20 minutes, rule changes means that only composer + yarn + performance tests run on that schedule, and everything except performance tests run on commit.
https://git.drupalcode.org/project/core_performance_testbed/-/pipelines
Because this also includes the OTEL_COLLECTOR change, it's feeding data into the graphs too, so you get a better idea what they'll look like:
http://grafana.tag1demo.tag1.io/d/teMVIdjVz/umami?orgId=1&refresh=30s&fr...
This means I've removed the manual job from this MR, since while it would be nice to add that eventually, it'll be a follow-up once we've got branches and commit hashes into traces from gitlab (which depends on this MR...).
Comment #11
catchSelf review.
There are some fixes to PerformanceTestTrait in here.
The change from
$_ENVtogetenv()- this is because $_ENV can't see the cli argument passed in to phpunit.Checking we have a first contentful paint before exiting polling, and ensuring at least two iterations. This was to fix some bad data (about 2 or 3 requests) I saw in traces from runs on gitlab which I haven't seen locally even once - we were logging a largestContentfulPaint event without logging a firstContentfulPaint event. It's possible that when they are the same browser event (the first is also the largest), that chrome can put LCP in the logs before FCP, then if we stop polling at that moment, we never get the corresponding FCP event.
edit: I had a workaround for a weird chromedriver timestamp discrepancy in the original patch, but was able to find the same information in different log entries. This considerably simplifies the current logic and should be considerably more robust (I hope) but makes the diff a bit bigger.
I will probably open a new issue to add some assertions in here (like is lcp greater or equal to fcp) so we can hard fail if we get bad data which should help to pick things up and also stop chromedriver log race conditions (or quasi race conditions) from adding bad data to the graphs. However, also don't want to introduce new random test failures to core, so would rather see how things go once this is actually up and running before doing that.
Was also debating whether to split the PerformanceTestTrait fixes here out to a separate issue, but these are massively intertwined at the moment - we need the gitlab runs to push data to Grafana, which will in turn show us how reliable the data on Grafana is, which will likely result in making the data collection more robust, which will then allow further improvements to Grafana (like potentially show mean requests per hour in the graph instead of per-data point).
I've added a skipped deprecation for the @return thing from Symfony which is in-line with what we're doing elsewhere.
The last thing is that currently core's daily test runs use "1" and the new performance jobs I added on my fork use "TRUE". Gitlab rules only accept string comparisons, no int or bool, so whatever we use it has to be a string. I do not have a strong preference, but we might want to pick one.
Have also set up a fake daily test job on the fork (it actually runs monthly, but you can press 'play' to simulate), to ensure that the daily tests will continue to work with these pipeline changes.
Comment #12
catchThe graphs were showing an issue I thought I had found a workaround for testing locally with the original MR.
The chrome performance log uses (at least) four different timestamp formats, two of which count from OS boot, and appear to start a few milliseconds away from each other.
This meant it was incredibly difficult to compare entries from the network vs. timeline logs and I had to do a rosetta stone kludge with a common event to calculate the offset from the two supposedly identical timestamps. The symptom was sometimes the request timestamp would be 'after' the response timestamp, giving you negative length spans, which Open Telemetry doesn't understand and Grafana graphs as lasting hundreds of years, looks funny but not very useful. However, I was getting the original symptom back when running the tests from gitlab, suggesting my workaround was not complete, might be an OS difference, or might be I just didn't have enough sample runs locally, or the tests were slow enough on my laptop it didn't hit negative span lengths.
When looking through to see if there was a better workaround, I found entries in the timeline log itself for the initial request and response, we are still dealing with four almost undocumented timestamp formats and some offset calculation, but I think it is at least four different kinds of fruit instead of a mixture of real and wooden fruit (apples to apples doesn't cover the improved situation). With this change, no weird negative timestamps and all the timings look about what I'd expect. I have a bit of a question mark over the node/1 hot cache being so much slower than the front page one, but given every is ~10% consistent that looks like something to investigate manually in a follow-up and not a symptom of the performance log parsing to me.
I still would prefer to commit this in one go, because the graphs validate the logic changes and the logic changes produce the graphs, but happy to split the test vs. YAML changes if it helps this land easier.
Comment #13
catch.
Comment #14
smustgrave commentedSo confirmed the pipeline runs on intervals by checking https://git.drupalcode.org/project/core_performance_testbed/-/pipeline_s...
See the results get sent to http://grafana.tag1demo.tag1.io/d/teMVIdjVz/umami?orgId=1&refresh=30s
But think something is off as the metrics appear to be empty.
Comment #15
catchIt's because I reduced the frequency of the pipeline from every 20 minutes to one hour. Bumped it back to 20 minutes and the graphs are populated again. We might want to figure out a lower-frequency schedule and tweak the graphs to still show something, but that'll be easier to do once this is in.
Comment #16
smustgrave commentedYup now I'm seeing the metrics.
Comment #17
catchFound a way to keep the graphs populated without running the pipeline every 20 minutes - the pipeline now repeats the tests three times in a loop (this is enough for prometheus to think it's got enough for a histogram is my theory), and this appears to allow us to then run the pipeline at any frequency. This should help us not blow the DA gitlab budget.
Also detecting when the chromedriver log is funky (which seems to be about 1/30 runs) and skipping the test when that happens instead of failing hard. Back to needs review for those couple of changes.
Comment #18
smustgrave commentedStill seeing the graphs populate.
Comment #20
catch+1 to the YAML changes, reduces repetition and removes a double-negative.
Comment #21
alexpottRe splitting this into trait fixes and yaml fixes. I don't think that that is worth it because as you point out - everything is going hand in hand. I think that performance testing in core is young enough to have a slightly looser scope than other core changes. Also this does not affect a running site so we should not be as cautious here.
Comment #22
alexpott@catch tested the gitlab updates on the fork of core and they work as expected. Let's get this in.
Committed and pushed e242a5aad6c to 11.x and c66bb852c6d to 10.2.x. Thanks!
Backported to 10.2.x because this is not runtime code and having this in 10.2.x will allow us to compare 10.2 and 11.x (and 10.3.x)