Skip to content

Functions Console Logs Capture #3656

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

Merged
merged 18 commits into from
Aug 16, 2022
Merged

Functions Console Logs Capture #3656

merged 18 commits into from
Aug 16, 2022

Conversation

lohanidamodar
Copy link
Member

@lohanidamodar lohanidamodar commented Aug 10, 2022

What does this PR do?

  • Captures and stores console logs from user functions in runtimes

Test Plan

Related PRs and Issues

(need this released with v2 runtimes tag so we can run CI tests)

Have you read the Contributing Guidelines on issues?

YES

@lohanidamodar lohanidamodar changed the title update container to accept new response format Functions Console Logs Capture Aug 10, 2022
Copy link
Member

@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

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

Looks good @lohanidamodar .

I would also like this to be tested in a concurrent setting as @stnguyen90 mentioned. You can have a simple script to create a series of simultaneously executions.

Within the cloud function you can console.log the current timestamp.
Within the logs, you can then check that no two executions have the same timestamp

@eldadfux
Copy link
Member

Let's give the links a bit more space so they can be in one line.
image

We should also change:
Output -> Stdout
Errors -> Stderr

@eldadfux
Copy link
Member

Also in the table Runtime should be just Time

image

@lohanidamodar
Copy link
Member Author

Let's give the links a bit more space so they can be in one line. image

We should also change: Output -> Stdout Errors -> Stderr

The output is actual response
log is the stdout
and error is the stderr

@eldadfux
Copy link
Member

Let's give the links a bit more space so they can be in one line. image
We should also change: Output -> Stdout Errors -> Stderr

The output is actual response log is the stdout and error is the stderr

We should fix it, but if that's the case where are the logs?

@lohanidamodar
Copy link
Member Author

Let's give the links a bit more space so they can be in one line. image
We should also change: Output -> Stdout Errors -> Stderr

The output is actual response log is the stdout and error is the stderr

We should fix it, but if that's the case where are the logs?

Not sure what you mean, here

  1. Output button (renamed to Response) => shows the response from user's code (execution.response)
  2. Logs (Renamed to Stdout) => shows the user's code's console logs (execution.stdout)
  3. error button (Renamed to Stderr) => shows the execution errors (execution.stderr)

@lohanidamodar
Copy link
Member Author

Looks good @lohanidamodar .

I would also like this to be tested in a concurrent setting as @stnguyen90 mentioned. You can have a simple script to create a series of simultaneously executions.

Within the cloud function you can console.log the current timestamp. Within the logs, you can then check that no two executions have the same timestamp

@christyjacob4 I will try, however as our runtimes are web server, which should handle concurrent requests already in correct sequence and output is buffered inside the execution not from the docker logs so we should be safe as far s I know.

@christyjacob4
Copy link
Member

@lohanidamodar we can potentially avoid patch releases if we test this scenario well...
So let's test it instead of giving rise to bugs post release. 👍

Copy link
Member

@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

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

The PR looks good. Just need to make final changes to the UI and try and hide the stdout attribute from the client response

@christyjacob4
Copy link
Member

@christyjacob4 christyjacob4 marked this pull request as ready for review August 15, 2022 22:39
@christyjacob4 christyjacob4 changed the base branch from master to 0.16.x August 15, 2022 22:39
Copy link
Member

@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

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

Looks good. Just some final changes and then we can merge.

We also need to update the appwrite/runtimes version in this file

Copy link
Member

@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

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

Awesome work 👌

Copy link
Member

@christyjacob4 christyjacob4 left a comment

Choose a reason for hiding this comment

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

Looks good. Just one comment.

@eldadfux eldadfux merged commit 69a4f51 into 0.16.x Aug 16, 2022
@stnguyen90 stnguyen90 deleted the feat-functions-console-logs branch January 26, 2023 01:34
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.

4 participants