fix: Need a way to disable flushing by losalex · Pull Request #1206 · googleapis/java-logging (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation37 Commits11 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

losalex

@losalex

@losalex

@losalex

@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

@losalex

@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

@losalex

@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

Choose a reason for hiding this comment

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

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

@losalex

minherz

Choose a reason for hiding this comment

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

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

@losalex

@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

@losalex

@losalex

daniel-sanche

svc-squareup-copybara pushed a commit to cashapp/misk that referenced this pull request

Oct 1, 2024

@github-cash-renovate-jvm-2 @svc-squareup-copybara

Package Type Package file Manager Update Change
com.google.cloud:google-cloud-logging
dependencies misk/gradle/libs.versions.toml gradle major
2.3.2 -> 3.20.2

Release Notes

googleapis/java-logging (com.google.cloud:google-cloud-logging)

v3.20.2

Dependencies

(31ec2b9)

v3.20.1

Dependencies

(dbd050c)

v3.20.0

Features

(034b9c4)

Dependencies

(cb6de76)

Documentation

(e3c6670)

v3.19.0

Features

(67db829)

Dependencies

(cb428d1)

v3.18.0

Features

(#​1615) (dc00cd0)

Dependencies

(9db8f3b)

v3.17.2

Dependencies

(e7a0904)

v3.17.1

Dependencies

(ea0db35)

(16d6192)

(9eb8834)

(16967e5)

v3.17.0

Features

(1dd64d0)

Bug Fixes

(848418b)

Dependencies

(edcaf8d)

v3.16.3

Dependencies

(8eb0781)

v3.16.2

Dependencies

(d52e623)

(81aa3e6)

v3.16.1

Bug Fixes

(15b05fc)

Dependencies

(6c5464d)

v3.16.0

Features

(9cd6b96)

Dependencies

(7fde779)

(b3e4f9b)

(f27713e)

(af784bc)

(20981dc)

v3.15.17

Dependencies

(235f1aa)

Documentation

(0440fc6)

v3.15.16

Bug Fixes

(e2f574c)

(9ef4d90)

Dependencies

(b40e846)

(30ba9ed)

v3.15.15

Bug Fixes

(c7a20de)

(21e1929)

Dependencies

(6cce3c9)

v3.15.14

Dependencies

(f3227db)

v3.15.13

Dependencies

(5835a7d)

(debc77f)

v3.15.12

Dependencies

(dc25a87)

(3080cec)

v3.15.11

Dependencies

(748e8a2)

v3.15.10

Dependencies

(e9e9835)

(9e750a3)

(7c2aa2c)

v3.15.9

Dependencies

(4f82f33)

(f931544)

(ff581a6)

(bd9be4e)

v3.15.8

Dependencies

(f9af381)

(0487cdf)

v3.15.7

Dependencies

(1a29b9d)

(f15d246)

v3.15.6

Dependencies

(03179b0)

v3.15.5

Dependencies

(8241302)

v3.15.4

Dependencies

(dce3c4c)

(a15c73c)

v3.15.3

Bug Fixes

(8138f46)

Dependencies

(8cd2a53)

v3.15.2

Dependencies

(b2f1111)

(b3b9d5f)

v3.15.1

Dependencies

(235f1aa)

Documentation

(0440fc6)

v3.15.0

Features

(7d43b80)

Dependencies

(dfb98f4)

v3.14.9

Dependencies

(5a56f1b)

(5aef8d6)

(fc2d065)

v3.14.8

Dependencies

(973d260)

v3.14.7

Dependencies

(febcf49)

v3.14.6

Dependencies

(9fa6f05)

v3.14.5

Dependencies

(84d42ae)

v3.14.4

Dependencies

(58ac608)

(296cce1)

(6363196)

v3.14.3

Dependencies

(e196a80)

v3.14.2

Bug Fixes

(#​1256) (09eeff0)

Dependencies

(d4bc663)

v3.14.1

Dependencies

(fdf6b7a)

(e73a704)

v3.14.0

Features

(125d94c)

Dependencies

(d9a381a)

v3.13.7

Bug Fixes

(d18bbba)

v3.13.6

Bug Fixes

(662a439)

v3.13.5

Dependencies

(b54e015)

v3.13.4

Dependencies

(f104203)

(7785a7c)

v3.13.3

Bug Fixes

(a69fd5e)

(aa0c176)

v3.13.2

Dependencies

(8b00108)

(c884361)

v3.13.1

Bug Fixes

(04bb6c0)

(f74b86d)

(c3942ea)

Dependencies

(01ebe33)

(d4f17ab)

v3.13.0

Features

(0931446)

(9e75fe4)

Dependencies

(1830525)

v3.12.1

Bug Fixes

(123960a)

v3.12.0

Features

(35be8d1)

v3.11.10

Dependencies

(413fa54)

(df8bfbe)

(4836c7e)

(a13ef9f)

v3.11.9

Dependencies

(5bd000c)

v3.11.8

Dependencies

(ad33e92)

v3.11.7

Bug Fixes

(a6ff3c6)

v3.11.6

Bug Fixes

(70fc6ee)

v3.11.5

Dependencies

(17a6e26)

v3.11.4

Bug Fixes

(6d3cb5b)

v3.11.3

Dependencies

(c08c4da)

(50c979b)

v3.11.2

Dependencies

(d38e9e0)

v3.11.1

Dependencies

(413fa54)

(df8bfbe)

(4836c7e)

(a13ef9f)

v3.11.0

Features

(bfef3d1)

Bug Fixes

(fa1a18f)

v3.10.7

Bug Fixes

(79e9d8d)

v3.10.6

Dependencies

(0998b9b)

v3.10.5

Bug Fixes

(dda1d0a)

v3.10.4

Dependencies

(b2731c7)

v3.10.3

Bug Fixes

(342157f)

(2d57d78)

(8c9a218)

(cafafe4)

(5666ee0)

(79baac6)

(765dd89)

(a93ee23)

(49bf5b4)

Dependencies

(e5f0b55)

v3.10.2

Dependencies

(17efd5c)

(3856e4f)

(a94e428)

v3.10.0

Features

(2749974)

Documentation

(1512487)

Dependencies

(18acf1f)

(01d3213)

v3.9.0

Features

(#​962) (4edb7e4)

Dependencies

(c969b4c)

(c95840b)

v3.8.0

Features

(576a93e)

3.7.6 (2022-05-03)

Documentation

(934df5a)

(33ae197)

(75331a6)

3.7.5 (2022-04-15)

Dependencies

(074c6c7)

(2e4b4d0)

3.7.4 (2022-03-31)

Bug Fixes

(1adf867)

(da92518)

3.7.3 (2022-03-29)

Dependencies

(b42717e)

(6d6bbef)

3.7.2 (2022-03-24)

Documentation

(7e59bf3)

Dependencies

(b692e02)

(d834a0a)

(c1d6559)

(3c44e3e)

(3721ef5)

3.7.1 (2022-03-03)

Bug Fixes

(19e9abb)

Dependencies

(4885440)

v3.7.7

Dependencies

(feaf255)

v3.7.6

v3.7.5

v3.7.4

v3.7.3

v3.7.2

v3.7.1

v3.7.0

Features

(27c199b)

Bug Fixes

(5c97fea)

3.6.4 (2022-02-15)

Dependencies

(f3d6f3f)

3.6.3 (2022-02-11)

Dependencies

(2d05dc8)

3.6.2 (2022-02-03)

Dependencies

(#​851) (7a5ee11)

(286728a)

(eb2eef5)

3.6.1 (2022-01-18)

Bug Fixes

(99fb678)

Dependencies

(21c2436)

v3.6.4

v3.6.3

v3.6.2

v3.6.1

v3.6.0

Features

(bb25d5d)

3.5.3 (2022-01-07)

Dependencies

(2816bb3)

3.5.2 (2021-12-28)

Bug Fixes

(0150655)

(#​780) (3f70b62)

(62e7838)

(3083bca)

3.5.1 (2021-12-03)

Dependencies

(56907a4)

v3.5.3

v3.5.2

v3.5.1

v3.5.0

Features

(23f7fa5)

(86223ff)

Bug Fixes

(e8cf6f9)

(790fb1a)

Dependencies

(c003417)

v3.4.0

Features

(25673fd)

(43ea0b4)

(1fa3a6e)

Configuration

📅 Schedule: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻ Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this PR and you won't be reminded about this update again.

This PR has been generated by Renovate Bot.

GitOrigin-RevId: d258e2fb591810de162fa68bc12f5c5743b9cb2e

Labels

api: logging

Issues related to the googleapis/java-logging API.

size: m

Pull request size is medium.