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

Use coarsetime #92

Merged
merged 6 commits into from
Feb 21, 2025
Merged

Use coarsetime #92

merged 6 commits into from
Feb 21, 2025

Conversation

nickschuch
Copy link
Contributor

@nickschuch nickschuch commented Feb 20, 2025

What does this change

@nickschuch nickschuch added the performance-test-needed Will trigger the performance tests label Feb 20, 2025
@nickschuch nickschuch merged commit e8ab571 into main Feb 21, 2025
7 checks passed
@nickschuch nickschuch deleted the coarsetime branch February 21, 2025 09:54
event->start_time = ctx->PHP_FUNCTION_ARG_START_TIME;
event->end_time = ctx->PHP_FUNCTION_ARG_END_TIME;
event->timestamp = bpf_ktime_get_ns();
event->elapsed = ctx->PHP_FUNCTION_ARG_ELAPSED;
Copy link

Choose a reason for hiding this comment

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

That is actually clever.

Because you are now no longer affecting performance you can get the ts in the event instead of from the actual function in PHP. Event ordering is still giving the parent hierarchy, right?

Another way - though slightly more effort is to create a real realtime clock TS at start of request.

And then pass this realtime start, monotonic start and monotonic end always to the probe! - that way it’s both realtime and not really.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been working well. I really like decoupling the timestamps and letting the probe me a metric.

@@ -30,16 +30,16 @@ unsafe extern "C" fn execute_ex(execute_data: *mut sys::zend_execute_data) {
};

// Run the upstream function and record the duration.
let start = get_unix_timestamp_micros();
let start = Instant::recent();
Copy link

Choose a reason for hiding this comment

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

One problem is that you only update on function end. I don’t know how realistic it is to never call a small function that returns, but maybe a good way to solve this is to have a call stack depth % 20 or so and call update() every once in a while.

That can be a stack local variable - right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a look at options. Thanks for the review @LionsAd !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance-test-needed Will trigger the performance tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants