fix: Need a way to disable flushing

[losalex]

Fixes #1143 ☕️

[daniel-sanche]

Can you provide some more context to make review easier? It looks like you're trying to provide a way to avoid flushing logs. How does Severity.UNRECOGNIZED tie into this?

Note that this sounds very similar to a bug I addressed in the logback library a couple years ago. Does this tie in with that feature in any way?

[losalex]

Can you provide some more context to make review easier? It looks like you're trying to provide a way to avoid flushing logs. How does Severity.UNRECOGNIZED tie into this?

Note that this sounds very similar to a bug I addressed in the logback library a couple years ago. Does this tie in with that feature in any way?

I use Severity.UNRECOGNIZED as a severity to signal turning off flushing for java-logging since there is no way to extend equivalent LogSeverity which is protobuf enum . The fix you shared is related to java-logback and not related to java-logging which can be used standalone. Other than that the change is straightforward - exposing setters/getters and patching logic to avoid flushing

[daniel-sanche]

I use Severity.UNRECOGNIZED as a severity to signal turning off flushing for java-logging since there is no way to extend equivalent LogSeverity which is protobuf enum .

Overloading Severity.UNRECOGNIZED for this doesn't really sit right with me. It feels like it could cause significant technical debt in the future.

1. Can we use a separate bool flag to turn flushing on/off instead of overloading the Severity field?
2. Or, since everything seems to use the Severity enum, couldn't we add a Severity.OFF item there, without it having a corresponding proto entry? Maybe have Severity.OFF map to null?
3. It seems like this feature might already be in place if you pass null as flushSeverity? Is that an intended feature? Maybe we just need to document that better?

[losalex]

I use Severity.UNRECOGNIZED as a severity to signal turning off flushing for java-logging since there is no way to extend equivalent LogSeverity which is protobuf enum .

Overloading Severity.UNRECOGNIZED for this doesn't really sit right with me. It feels like it could cause significant technical debt in the future.

1. Can we use a separate bool flag to turn flushing on/off instead of overloading the Severity field?
2. Or, since everything seems to use the Severity enum, couldn't we add a Severity.OFF item there, without it having a corresponding proto entry?

I must admit I am not exactly following your suggestions above - the feature for flush severity is designed to flush pending writes if log severity is equal or above flush severity... LogSeverity.UNRECOGNIZED already exists and I use it only for this specific feature to indicate that we should skip logging since flush severity is Severity.UNRECOGNIZED - agree that it might not be great name, but I was thinking that creating Severity.OFF = LogSeverity.UNRECOGNIZED did not make much sense either since Severity is tied with LogSeverity.

[daniel-sanche]

pending writes if log severity is equal or above flush severity... LogSeverity.UNRECOGNIZED already exists and I use it only for this specific feature to indicate that we should skip logging since flush severity

My concern is that the spec for LogSeverity.UNRECOGNIZED was not intended to mean "log flusing off", so I'm worried about future problems from overloading the meaning of that field

but I was thinking that creating Severity.OFF = LogSeverity.UNRECOGNIZED did not make much sense either since Severity is tied with LogSeverity.

Could we do Severity.OFF = null instead, so we're not overloading an existing field?

[losalex]

pending writes if log severity is equal or above flush severity... LogSeverity.UNRECOGNIZED already exists and I use it only for this specific feature to indicate that we should skip logging since flush severity

My concern is that the spec for LogSeverity.UNRECOGNIZED was not intended to mean "log flusing off", so I'm worried about future problems from overloading the meaning of that field

but I was thinking that creating Severity.OFF = LogSeverity.UNRECOGNIZED did not make much sense either since Severity is tied with LogSeverity.

Could we do Severity.OFF = null instead, so we're not overloading an existing field?

pending writes if log severity is equal or above flush severity... LogSeverity.UNRECOGNIZED already exists and I use it only for this specific feature to indicate that we should skip logging since flush severity

My concern is that the spec for LogSeverity.UNRECOGNIZED was not intended to mean "log flusing off", so I'm worried about future problems from overloading the meaning of that field

but I was thinking that creating Severity.OFF = LogSeverity.UNRECOGNIZED did not make much sense either since Severity is tied with LogSeverity.

Could we do Severity.OFF = null instead, so we're not overloading an existing field?

My concern is that the spec for LogSeverity.UNRECOGNIZED was not intended to mean "log flusing off", so I'm worried about future problems from overloading the meaning of that field

Not sure I understand what you mean here - can you please let me know what is a meaning of LogSeverity.UNRECOGNIZED?

[daniel-sanche]

Unrecognized is part of the log severity proto definition. The spec doesn't make it clear how it is meant to be used, but that doesn't mean it's safe to redefine.

We will likely encounter some legitimate logs with their severity field set to "unrecognized". (speculation: maybe some OTel exporters will use "unrecognized" when there's not a direct mapping between a GCP LogSeverity and a different vendor's?) If we treat "Unrecognized" to mean "Log flushing Off", that could make things confusing if customers attempt to actually filter for those logs. And it could be hard to fix in the future

[losalex]

Unrecognized is part of the log severity proto definition. The spec doesn't make it clear how it is meant to be used, but that doesn't mean it's safe to redefine.

We will likely encounter some legitimate logs with their severity field set to "unrecognized". (speculation: maybe some OTel exporters will use "unrecognized" when there's not a direct mapping between a GCP LogSeverity and a different vendor's?) If we treat "Unrecognized" to mean "Log flushing Off", that could make things confusing if customers attempt to actually filter for those logs. And it could be hard to fix in the future

In order to redefine a meaning, we need to know the meaning... LogSeverity.UNRECOGNIZED used in library already today to indicate invalid severity levels and end up in exception (see here for example). Also it cannot be used to generate log entries due to same reason - severity UNRECOGNIZED is not supported. So I am less worried about exposing Severity.UNRECOGNIZED since we can clearly document that it cannot be used to generate log entries and if anyone uses it the exception will happen. And if my assumption is correct, seems Severity.UNRECOGNIZED will never be used to generate legit logs since it is common practice to define values in enum for invalid/error cases.
In general, we do not have an equivalent for Level.OFF in Severity and using LogSeverity.UNRECOGNIZED is probably the only option since Java does not allows enum inheritance...

[minherz]

Given the expectations to the backward compatibility and an attempt to avoid introduction of the new interfaces while keeping the existing interface intuitive, I propose the following implementation and recommend to extend it to the java-logging-logback library:

1. Refactor LoggingHandler.severityFor(Level level) method to switch/case based on the java.util.logging.Level values and not Integer values that supposed to reflect the numeric values of the logging levels.
2. For the case Level.OFF return null:

   case Level.OFF:
       return null;

3. Keep the rest implementation (I mainly reference to LoggingImpl.java:885) as it is now.

[daniel-sanche]

Given the expectations to the backward compatibility and an attempt to avoid introduction of the new interfaces while keeping the existing interface intuitive, I propose the following implementation and recommend to extend it to the java-logging-logback library:

1. Refactor LoggingHandler.severityFor(Level level) method to switch/case based on the java.util.logging.Level values and not Integer values that supposed to reflect the numeric values of the logging levels.
2. For the case Level.OFF return null:

   case Level.OFF:
       return null;

3. Keep the rest implementation (I mainly reference to LoggingImpl.java:885) as it is now.

Yeah, this seems like a great solution to me. It seems to be in line with the original intent of the setFlushSeverity function, where null was documented to mean "flush off", so this feels like the safest way to implement this change

[minherz]

very solid improvement compared to the previous push.
consider adding the mentioned null validations.
do you think adding a test that validates the exception if trying to write with Severity.NONE makes sense here?

[losalex]

very solid improvement compared to the previous push. consider adding the mentioned null validations. do you think adding a test that validates the exception if trying to write with Severity.NONE makes sense here?

Thanks! Added test to validate that Severity.NONE cannot be used

[losalex]

Given the expectations to the backward compatibility and an attempt to avoid introduction of the new interfaces while keeping the existing interface intuitive, I propose the following implementation and recommend to extend it to the java-logging-logback library:

1. Refactor LoggingHandler.severityFor(Level level) method to switch/case based on the java.util.logging.Level values and not Integer values that supposed to reflect the numeric values of the logging levels.
2. For the case Level.OFF return null:

   case Level.OFF:
       return null;

3. Keep the rest implementation (I mainly reference to LoggingImpl.java:885) as it is now.

Yeah, this seems like a great solution to me. It seems to be in line with the original intent of the setFlushSeverity function, where null was documented to mean "flush off", so this feels like the safest way to implement this change

As I mentioned before, I believe that using nullable enums is not a good practice. Added Severity.NONE