-
Notifications
You must be signed in to change notification settings - Fork 39
feat: Add StatusMessageWatcher
#793
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
base: master
Are you sure you want to change the base?
Conversation
StatusSessageWatcherStatusMessageWatcher
| "url": "https://github.com/apify/apify-client-js/issues" | ||
| }, | ||
| "homepage": "https://docs.apify.com/api/client/js/", | ||
| "files": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for testing, revert before merging.
| * Helper class for redirecting Actor run status and status message to log. | ||
| */ | ||
| export class StatusMessageWatcher { | ||
| private static finalSleepTimeMs = 6000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two constants are for sure up for discussion.
barjin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Pijukatel! I know it's already in Python Client, and there's an issue asking for this, but still - I'm not completely convinced we need this feature in the Client.
I have a few opinions, in no particular order:
- The status message is a "discrete" piece of data (unlike e.g.
log), so polling the API should be the user's job. - (If we still do this), we should allow for consuming the changed status without logging it (e.g.
run().onStatus((status) => { user code })) - (If we don't do this), could this be an option for the logger, e.g.
.getStreamedLog({ statusMessage: true })? Having a separate method for this doesn't make much sense to me.
cc @janbuchar @B4nan wdyt?
Anyway, regarding the code, a few comments from a quick look:
| const runData = await this.get(); | ||
| const runId = runData ? `${runData.id ?? ''}` : ''; | ||
|
|
||
| const actorId = runData?.actId ?? ''; | ||
| const actorData = (await this.apifyClient.actor(actorId).get()) || { name: '' }; | ||
|
|
||
| const actorName = runData ? (actorData.name ?? '') : ''; | ||
| const name = [actorName, `runId:${runId}`].filter(Boolean).join(' '); | ||
|
|
||
| return new Log({ level: LEVELS.DEBUG, prefix: `${name} -> `, logger: new LoggerActorRedirect() }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const runData = await this.get(); | |
| const runId = runData ? `${runData.id ?? ''}` : ''; | |
| const actorId = runData?.actId ?? ''; | |
| const actorData = (await this.apifyClient.actor(actorId).get()) || { name: '' }; | |
| const actorName = runData ? (actorData.name ?? '') : ''; | |
| const name = [actorName, `runId:${runId}`].filter(Boolean).join(' '); | |
| return new Log({ level: LEVELS.DEBUG, prefix: `${name} -> `, logger: new LoggerActorRedirect() }); | |
| const runData = await this.get(); | |
| const runId = runData?.id ?? ''; | |
| const actorId = runData?.actId ?? ''; | |
| const actorData = (await this.apifyClient.actor(actorId).get()) || { name: '' }; | |
| const actorName = actorData?.name ?? ''; | |
| const name = [actorName, `runId:${runId}`].filter(Boolean).join(' '); | |
| return new Log({ level: LEVELS.DEBUG, prefix: `${name} -> `, logger: new LoggerActorRedirect() }); |
This is the removed code from L282, maybe this PR was made before merging the final version of log streaming?
| if (newStatusMessage !== this.lastStatusMessage) { | ||
| // Log only when status or status message changed | ||
| this.lastStatusMessage = newStatusMessage; | ||
| this.toLog.info(newStatusMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idea: we might set the logging level based on the run status?

Description
Add the option to log status and status messages of another Actor run. If such Actor run is also redirecting logs from another Actor run, then those logs can propagate all the way to the top through the
StreamedLog- deep redirection. If the Actor run from which we are redirecting status messages is not redirecting logs from its children, then the log and status redirection remains shallow - only from that Actor run.Example usage:
Actor.call - default
This will redirect logs by default. Example default redirected line:
Actor.call - custom Log
This will redirect logs using custom log and logger. Example default redirected line:
Actor.call - no log redirection
This will disable all log redirection (same as current behavior)
Actor.run - attaching to already running actor and redirecting statuses and status messages
A typical use case is redirecting statuses and status messages from an Actor that runs in standby.
Example actor with recursive redirection:
https://console.apify.com/actors/IcfIKTIvbmujMmovj/source
Issues
.actor().call()method to set the correct timeout, show the progress in status message, and stream logs #632StatusMessageWatcherapify-client-python#407