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

Distinguish initial alarm invocations from retries. #1673

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

jqmmes
Copy link
Contributor

@jqmmes jqmmes commented Feb 15, 2024

This PR adds a new runAlarm parameter to keep track of alarm invocation retries.

@jqmmes jqmmes requested review from a team as code owners February 15, 2024 15:57
@jqmmes jqmmes requested review from fhanau and vickykont February 15, 2024 15:57
@jqmmes jqmmes force-pushed the joaquim/runalarm-retry branch from 3749029 to 8b888e3 Compare February 15, 2024 16:09
@jqmmes jqmmes changed the title Add a new parameter to distinguish initial runAlarm from retries. Distinguish initial alarm invocations from retries. Feb 15, 2024
Copy link
Member

@a-robinson a-robinson left a comment

Choose a reason for hiding this comment

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

This code does what it says, but what's the purpose? What's this value going to be used for?

src/workerd/io/worker-interface.capnp Outdated Show resolved Hide resolved
src/workerd/io/worker.c++ Outdated Show resolved Hide resolved
@a-robinson
Copy link
Member

This code does what it says, but what's the purpose? What's this value going to be used for?

And is there a reason for choosing a bool over a count of the number of past attempts?

@DaniFoldi
Copy link
Contributor

Hi 👋,

With my somewhat limited c++ knowledge, I couldn't find any references to the js invocation of the alarm handler - this would be useful to have exposed to the handler for observability purposes. Is that something that could be added perhaps? It's the user code throwing that causes a retry anyways, so in theory it could be tracked by user code (try/catch, store and rethrow), but a native counter would be more reliable.

@jqmmes
Copy link
Contributor Author

jqmmes commented Feb 15, 2024

This code does what it says, but what's the purpose? What's this value going to be used for?

And is there a reason for choosing a bool over a count of the number of past attempts?

We'll still record each and every invocation internally, I can make it as a counter if you think it's best. Then we would have something like:

invocation X retry 0
invocation X retry 1
invocation X retry 2
invocation X retry 3
...

Instead of

invocation X retry false
invocation X retry true
invocation X retry true
...

@jqmmes jqmmes force-pushed the joaquim/runalarm-retry branch 2 times, most recently from c0a10ef to 79fdf2b Compare February 16, 2024 15:56
@jqmmes
Copy link
Contributor Author

jqmmes commented Feb 16, 2024

PR Updated:

  • Applied reviewer suggestions.
  • Converted retry info from bool to uint32_t counting the current retry number (0 if it's not a retry).
  • Rebased

src/workerd/io/worker-interface.capnp Outdated Show resolved Hide resolved
src/workerd/server/alarm-scheduler.c++ Outdated Show resolved Hide resolved
@MellowYarker
Copy link
Contributor

With my somewhat limited c++ knowledge, I couldn't find any references to the js invocation of the alarm handler - this would be useful to have exposed to the handler for observability purposes. Is that something that could be added perhaps? It's the user code throwing that causes a retry anyways, so in theory it could be tracked by user code (try/catch, store and rethrow), but a native counter would be more reliable.

@DaniFoldi the alarm() handler is declared in global-scope.h and we invoke it in global-scope.c++, but yes this is a good idea.

@jqmmes jqmmes force-pushed the joaquim/runalarm-retry branch 3 times, most recently from eb7d65f to 966ecdc Compare February 22, 2024 10:38
src/workerd/io/worker-interface.capnp Outdated Show resolved Hide resolved
src/workerd/server/alarm-scheduler.c++ Outdated Show resolved Hide resolved
src/workerd/server/alarm-scheduler.c++ Outdated Show resolved Hide resolved
@jqmmes jqmmes force-pushed the joaquim/runalarm-retry branch 4 times, most recently from 5ba2b7f to 0aed909 Compare February 27, 2024 09:12
@jqmmes jqmmes force-pushed the joaquim/runalarm-retry branch from 0aed909 to 7014c32 Compare February 27, 2024 10:45
@jqmmes jqmmes merged commit 4cf7759 into main Feb 28, 2024
10 of 11 checks passed
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.

5 participants