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

[FEATURE REQUEST]: Allow injection of custom LoggerService #473

Open
thrixton opened this issue Apr 3, 2020 · 10 comments · May be fixed by #542
Open

[FEATURE REQUEST]: Allow injection of custom LoggerService #473

thrixton opened this issue Apr 3, 2020 · 10 comments · May be fixed by #542
Labels
enhancement New feature or request

Comments

@thrixton
Copy link
Contributor

thrixton commented Apr 3, 2020

It's not possible to redirect all logging to the applications own logging pipeline currently as both LoggerServiceFactory and ILoggerService are internal.

I would like to inject my own logging service (either seq or Application Insights depending on environment) and have all logs consolidated.

It seems log4net is suppoerted via config file however I have other correlation information that I need to add which I cannot see how to accomplish.

Is this possible or is there another way to achieve this?

@thrixton thrixton added the enhancement New feature or request label Apr 3, 2020
@imback82
Copy link
Contributor

imback82 commented Apr 3, 2020

We could expose LoggerServiceFactory and ILoggerService as public, but let me double-confirm. Meanwhile, you can use https://github.com/aelij/IgnoresAccessChecksToGenerator to access internal classes if you are blocked.

@thrixton thrixton linked a pull request Jun 9, 2020 that will close this issue
@thrixton
Copy link
Contributor Author

@imback82
I exposed this via https://github.com/aelij/IgnoresAccessChecksToGenerator as per your suggestion, and then called the static LoggerServiceFactory.SetLoggerService method, passing my custom logger, but no logs are generated.
The problem appears to be that some (maybe all) logger instances are created before my application is initialized and I set the logger.
Do you have any suggestions as to the best way to override logging?

@imback82
Copy link
Contributor

Do you set it before executing any Spark related code? The following works for me.

        public void Run(string[] args)
        {
            LoggerServiceFactory.SetLoggerService(MyLogger.s_instance);

            SparkSession spark = SparkSession
                .Builder()
                .AppName(".NET Spark SQL basic example")
                .Config("spark.some.config.option", "some-value")
                .GetOrCreate();

            spark.Stop();
        }

@imback82
Copy link
Contributor

By the way, this applies only to the driver side log, not the worker. So, #542 may not be a good long term solution. Integrating with log4net or something similar will be good so that you can get a consistent experience both on the driver and worker side.

@thrixton
Copy link
Contributor Author

@imback82 , yes, this was a user error sorry.
Race condition between creating the session vs setting the logger, oops :)
Thanks

Re #542, you're right, so i'll have a look at that option.
Would it be possible to replace log4net with a more modern and maintained logging framework (serilog maybe)?
Log4net appears to be "dormant".
I currently use Serilog pushing either to Seq or application insights which works well.

@imback82
Copy link
Contributor

Would it be possible to replace log4net with a more modern and maintained logging framework (serilog maybe)?

serilog should be good. Do you want to work on this? If so, I can break down work items for you. Thanks!

@thrixton
Copy link
Contributor Author

Happy to implement this.
If you break down the work items i'll work through them.

@imback82
Copy link
Contributor

Cool. Here is what I am thinking:

  1. Implement ILoggerService with serilog.
  2. Update SparkEnvironment to set a logger based on an environment variable. Note that the work side should also pick up this environment variable (check CreateEnvVarsForPythonFunction) and create the same logger.
  3. Document how to utilize serilog under docs.
  4. Possibly make this as a default logger instead of ConsoleLoggerService.

You can think of each bullet as a separate PR. Thanks in advance!

@suhsteve
Copy link
Member

Is there any progress on this work ?

I think a good use case: Microsoft.Spark library + notebooks + dotnet repl. It'd be good to redirect logger messages elsewhere in order to not pollute the notebook cell outputs.

ie
image

@thrixton
Copy link
Contributor Author

No progress as yet sorry, I've had to redirect my efforts in the (not so) short term.
I'll see if I can find some time over the next couple of weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants