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

Configure loggers for function #1421

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

akihikokuroda
Copy link
Collaborator

Summary

"user" and "function" loggers are created in the runner class
The "user" logger writes the log into /data/{job_id}/user.log file which is /{user_name}/{job_id}/user.log in the persistent storage
The "function" logger writes the log into /function_date/{job_id}/function.log file which is /{provider_name}/{job_id}/function.log in the persistent storage

The function code can get these logger by logger.getLogger("user") and `logger.getLogger("function")

An optional argument type is added to job.logs() method. The 'type value should be "user" or "function".
For the provider, it returns both "user" and "function" logs. For the user, it returns only "user" log

Details and comments

@akihikokuroda akihikokuroda marked this pull request as draft July 24, 2024 21:14
@akihikokuroda
Copy link
Collaborator Author

This PR has dependency on #1408

@akihikokuroda akihikokuroda marked this pull request as ready for review July 27, 2024 16:41
@Tansito Tansito added the on-hold On hold due to any reason label Jul 29, 2024
@Tansito
Copy link
Member

Tansito commented Jul 29, 2024

@akihikokuroda let me put this PR on-hold until we release 0.15. I think this PR will require time to discuss it and we have more urgent priorities for that release 🙏

@Tansito Tansito requested a review from IceKhan13 August 7, 2024 17:34
@Tansito Tansito removed the on-hold On hold due to any reason label Aug 7, 2024
@Tansito
Copy link
Member

Tansito commented Aug 7, 2024

Removing the label now that we released the new version and we can discuss this calmly 😄

Copy link
Collaborator

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

I think that with the new catalog client, we may be able to tackle the logging issue there and leave the original client as is (?)

Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

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

I like this approach @akihikokuroda , thank you. Some comments that I have about the implementation:

  • log_type: I don't think is needed but correct me if I'm wrong, Aki. Backend logic can be modified to identify if the function is from provider or user without the need to use this variable.
  • I see that this is mostly for provider functions. How could we do this for user functions?
  • Right now we have some validations in the logs like this one, how can we check these things with this new approach?

@akihikokuroda
Copy link
Collaborator Author

log_type: I don't think is needed but correct me if I'm wrong, Aki. Backend logic can be modified to identify if the function is from provider or user without the need to use this variable.

The provider can get 3 different logs (user, function and console output).
The user can get only the user log when the provider function is executed
The user can get 3 different logs (user, function and console output) when the user function is executed.

I see that this is mostly for provider functions. How could we do this for user functions?

The user can put the log entries into the different logs and retrieve them separately.

Right now we have some validations in the logs like this one, how can we check these things with this new approach?

It is using the standard logger function, so if log entries are not there, it returns nothing.

@Tansito
Copy link
Member

Tansito commented Aug 9, 2024

The provider can get 3 different logs (user, function and console output).
The user can get only the user log when the provider function is executed
The user can get 3 different logs (user, function and console output) when the user function is executed.

Well, this workflow is something I would like to validate with Sanket too so let's discuss it internally. Personally I don't see the value to support both kind of logs: user log and console output. Same as that provider would have access to user logs having them their own logs.

The user can put the log entries into the different logs and retrieve them separately.

For me as user sounds confusing to have two kind of logs that basically do the same.

It is using the standard logger function, so if log entries are not there, it returns nothing.

Before merging anything we need to ensure that we are not loosing any feature like that mentioned.

Let me Monday write in our internal issue about this to initiate a conversation with Paco and Sanket.

@Tansito Tansito marked this pull request as draft August 9, 2024 19:27
@akihikokuroda
Copy link
Collaborator Author

Well, this workflow is something I would like to validate with Sanket too so let's discuss it internally. Personally I don't see the value to support both kind of logs: user log and console output. Same as that provider would have access to user logs having them their own logs.

The use log entries are generated by the function that are shared with the user. It's still valuable for the provider to check what are shared with the user.
The console output may include the stack back trace when some exception happens. The function developer must want to see them :-)

@Tansito
Copy link
Member

Tansito commented Aug 9, 2024

The use log entries are generated by the function that are shared with the user. It's still valuable for the provider to check what are shared with the user.

I agree but I feel a bit uncomfortable doing that so I prefer to confirm that that's exactly what we are looking for. For that is for what we are separating logs but I agree with you in this assumption. Logs management is something that it's not closed yet and I prefer to think well about it taking into account that it's not so urgent.

The console output may include the stack back trace when some exception happens. The function developer must want to see them :-)

Can we not redirect that to the logs? Thinking in that at some point as we talked we were going to remove logs from database (something that I think it has a lot of sense).

@akihikokuroda
Copy link
Collaborator Author

Can we not redirect that to the logs? Thinking in that at some point as we talked we were going to remove logs from database (something that I think it has a lot of sense).

That's the original log. We can take it out but it probably contains critical information to debug the function issues.

@Tansito
Copy link
Member

Tansito commented Aug 9, 2024

Oh yes, I was thinking in that "function" logs not "user" logs. "user" logs should only print that information specifically sent it to it I suppose.

@akihikokuroda
Copy link
Collaborator Author

@Tansito Is there any progress for this? What is the next step?

@Tansito
Copy link
Member

Tansito commented Aug 19, 2024

Sorry @akihikokuroda I couldn't address this yet. Feel free to open the conversation in our internal issue. I will try to add there my comments.

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.

3 participants