Skip to content

Construct static Logging utitity class#82

Merged
gselzer merged 2 commits intomainfrom
scijava/scijava-log2/static-logger
Oct 16, 2023
Merged

Construct static Logging utitity class#82
gselzer merged 2 commits intomainfrom
scijava/scijava-log2/static-logger

Conversation

@gselzer
Copy link
Copy Markdown
Member

@gselzer gselzer commented Jul 25, 2023

This prevents us from having to pass a Logger around everywhere!

In addition, I noticed that the Logger and OpMonitor utility classes were still hanging around in the engine, so I removed them!

Closes scijava/scijava#106, scijava/scijava#13 and maybe other issues.

@gselzer gselzer added the enhancement New feature or request label Jul 25, 2023
@gselzer gselzer self-assigned this Jul 25, 2023
@gselzer
Copy link
Copy Markdown
Member Author

gselzer commented Jul 25, 2023

With e000bd7, we can also close scijava/scijava#64

@gselzer gselzer linked an issue Jul 25, 2023 that may be closed by this pull request
@gselzer
Copy link
Copy Markdown
Member Author

gselzer commented Jul 25, 2023

Over in scijava/scijava#13, @ctrueden said:

here are valid reasons for wanting to log, for example, only a single op at debug, but everything else at info or warn, because you really just want to debug that one op implementation. Being able to do this without changing/rebuilding any code can be really important in some deployment scenarios.

With this PR, you'd do something like the following:

var theExistingLogger = Logging.getLogger();
Logging.setLogger(myVeryVerboseLogger);
ops.op("some op I want to debug").arityX().inputs(...).apply();
Logging.setLogger(theExistingLogger);

What would you prefer? Something like:

var op = ops.op("some op I want to debug").arityX().inputs(...).function();
Logging.setLogger(op, myVeryVerboseLogger);
op.apply(...)

I don't feel too strongly one way or the other - honestly, it wouldn't be too terribly hard to add the other, if that's what you think people will use.

There are also valid reasons for wanting to have a different logger used per OpEnvironment, in case you have some complicated server-side JVM running multiple separate contexts.

This seems like something that the AbstractRichOp could be responsible for doing. Do you really foresee someone doing this? I'm happy to punt on this until it's needed.

From an architecture standpoint, I can't say that I love having a static Logging utility class—I think it limits the utility of logging—and we might be better off just going with a "standard best practice" logging approach that doesn't try to be too clever about reducing boilerplate. E.g. just suck it up and put the SLF4J Logger declaration at the top of each class that wants to do logging, I dunno...

Isn't this basically what we're doing now?

@gselzer gselzer requested a review from ctrueden July 25, 2023 21:15
@gselzer gselzer marked this pull request as ready for review July 25, 2023 21:15
@gselzer gselzer requested a review from hinerm July 25, 2023 21:15
@hinerm hinerm force-pushed the scijava/scijava-log2/static-logger branch 3 times, most recently from c386f6d to 08218bc Compare August 11, 2023 18:02
@hinerm
Copy link
Copy Markdown
Member

hinerm commented Aug 11, 2023

Discussion in scijava/scijava#13 looks unresolved. I'm going to move this to a draft PR and remove the reviewers for the moment until we have a chance to discuss further.

@hinerm hinerm removed request for ctrueden and hinerm August 11, 2023 19:20
@hinerm hinerm marked this pull request as draft August 11, 2023 19:20
@gselzer gselzer linked an issue Sep 26, 2023 that may be closed by this pull request
@gselzer gselzer force-pushed the scijava/scijava-log2/static-logger branch 6 times, most recently from 11a45b5 to 453608a Compare October 16, 2023 18:51
@gselzer gselzer force-pushed the scijava/scijava-log2/static-logger branch from 453608a to afb9e08 Compare October 16, 2023 18:59
We aren't using it anymore, but we can bring it back later if needed
@gselzer gselzer marked this pull request as ready for review October 16, 2023 19:13
@gselzer gselzer merged commit 4ef7e39 into main Oct 16, 2023
@gselzer gselzer deleted the scijava/scijava-log2/static-logger branch October 16, 2023 19:15
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

No open projects

2 participants