search

apache / logging-log4j-kotlin

A Kotlin-friendly interface to log against the Log4j API
https://logging.apache.org/log4j/kotlin
Apache License 2.0
45 stars 11 forks source link

Logging context convenience functions for coroutines #65

Closed rocketraman closed 4 months ago

rocketraman commented 5 months ago

We were already able to create logging context for coroutines, but it was more verbose than it should be (https://github.com/apache/logging-log4j-kotlin/issues/64). Create some convenience functions to make it easy to set (or add to) the logging context for a coroutine.

Resolves https://github.com/apache/logging-log4j-kotlin/issues/64.

vy commented 5 months ago

@rocketraman, the CI is failing, could you also fix the build too, please?

rocketraman commented 5 months ago

@vy Have made the updates you suggested, please re-review.

Regarding the build, I'm not sure what is going on there. Seems to be the bnd plugin reporting that a major semver update is required. The changes I've made shouldn't violate that requirement -- no existing signatures were changed or removed. Looking at the baseline report, it seems to think that the hash differences in some resources are violating the major semver requirement, but that seems silly:

    MAJOR              RESOURCES  <resources>
      MAJOR            RESOURCE   META-INF/DEPENDENCIES
        ADDED          SHA        E61DD8F862326C5F9364CF8D0F0364716AFD0A5F
        REMOVED        SHA        EFCC6FAB931F5DE508C45E8FA038B661AF5F5964
      MAJOR            RESOURCE   META-INF/log4j-api-kotlin.kotlin_module
        ADDED          SHA        6E4B3B3DEB35CA98C4A570172B2E9D95576F87F3
        REMOVED        SHA        896123946070B5451C90650D412B3C855B064A11
      MAJOR            RESOURCE   META-INF/maven/org.apache.logging.log4j/log4j-api-kotlin/pom.properties
        ADDED          SHA        0F49081B0127CA3A7AC3DB5853BB02E1172D8E8D
        REMOVED        SHA        6E763BC80951CD200A6DCBAFA5D8D4F227BC43E3
      MAJOR            RESOURCE   META-INF/maven/org.apache.logging.log4j/log4j-api-kotlin/pom.xml
        ADDED          SHA        C929BE3B7F195D63E13EC46AFF00FE62A3B7D7A7
        REMOVED        SHA        CBB42A3C296605A637E83378D925A25F964543FB
      MAJOR            RESOURCE   module-info.class
        ADDED          SHA        0B6329B7A62E27F2A18269B553B566A11DA89C0B
        REMOVED        SHA        170B4D950889694286FBBCB7C79D9EF1BBBB0017
      MAJOR            RESOURCE   org/apache/logging/log4j/kotlin/CoroutineThreadContext$Key.class
        REMOVED        SHA        37F35F6B1F895D2217B0914266413A5E8104AB81
        ADDED          SHA        63CF843F979424BB33719BE6C1082ABC63074E18
      MAJOR            RESOURCE   org/apache/logging/log4j/kotlin/CoroutineThreadContext.class
        REMOVED        SHA        86D0417B446E3563893B1B8210907FB5560872C5
        ADDED          SHA        A86A59ECEBC85FD3C61D3548C1A5CAE260B3A031
      ADDED            RESOURCE   org/apache/logging/log4j/kotlin/CoroutineThreadContextKt.class
        ADDED          SHA        18E42936D4876837354C75AE60D4AE39FE61BCC8
      MAJOR            RESOURCE   org/apache/logging/log4j/kotlin/SimpleLoggerLruCache.class
        ADDED          SHA        64DFFC834B2D657B604DEB42132B1D7594C3A87C
        REMOVED        SHA        75E35DEE03546075ACD6D1B3E0D939465D994CF9
      MAJOR            RESOURCE   org/apache/logging/log4j/kotlin/ThreadContextData.class
        REMOVED        SHA        7D10FA06483D0150C5BA7E5518BC434ACFBF9518
        ADDED          SHA        A19B02941F23808986A7F5B6801750D40499DDE1

Perhaps we use https://github.com/bndtools/bnd/tree/master/maven-plugins/bnd-baseline-maven-plugin#diffignores?

ppkarwasz commented 5 months ago

@rocketraman,

You need to look at the end of the BND report:

[ERROR] * org.apache.logging.log4j.kotlin                    PACKAGE    MINOR      1.4.1      1.4.0      1.5.0      -
[ERROR]   MINOR                PACKAGE    org.apache.logging.log4j.kotlin
[ERROR]     ADDED              CLASS      org.apache.logging.log4j.kotlin.CoroutineThreadContextKt
[ERROR]       ADDED            ACCESS     final
[ERROR]       ADDED            METHOD     additionalLoggingContext(java.util.Map,java.util.Collection)
[ERROR]         ADDED          ACCESS     static
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.NotNull
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          RETURN     org.apache.logging.log4j.kotlin.CoroutineThreadContext
[ERROR]       ADDED            METHOD     clone()
[ERROR]         ADDED          ACCESS     protected
[ERROR]         ADDED          ANNOTATED  jdk.internal.vm.annotation.IntrinsicCandidate
[ERROR]         ADDED          RETURN     java.lang.Object
[ERROR]       ADDED            METHOD     equals(java.lang.Object)
[ERROR]         ADDED          RETURN     boolean
[ERROR]       ADDED            METHOD     finalize()
[ERROR]         ADDED          ACCESS     protected
[ERROR]         ADDED          RETURN     void
[ERROR]       ADDED            METHOD     hashCode()
[ERROR]         ADDED          ANNOTATED  jdk.internal.vm.annotation.IntrinsicCandidate
[ERROR]         ADDED          RETURN     int
[ERROR]       ADDED            METHOD     loggingContext(java.util.Map,java.util.Collection)
[ERROR]         ADDED          ACCESS     static
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.NotNull
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          RETURN     org.apache.logging.log4j.kotlin.CoroutineThreadContext
[ERROR]       ADDED            METHOD     toString()
[ERROR]         ADDED          RETURN     java.lang.String
[ERROR]       ADDED            METHOD     withAdditionalLoggingContext(java.util.Map,java.util.Collection,kotlin.jvm.functions.Function2,kotlin.coroutines.Continuation)
[ERROR]         ADDED          ACCESS     static
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.NotNull
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.NotNull
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          RETURN     java.lang.Object
[ERROR]       ADDED            METHOD     withLoggingContext(java.util.Map,java.util.Collection,kotlin.jvm.functions.Function2,kotlin.coroutines.Continuation)
[ERROR]         ADDED          ACCESS     static
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.NotNull
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.NotNull
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.Nullable
[ERROR]         ADDED          RETURN     java.lang.Object
[ERROR]       ADDED            ANNOTATED  kotlin.Metadata
[ERROR]         ADDED          PROPERTY   d1.0='...'
[ERROR]         ADDED          PROPERTY   d2.0='additionalLoggingContext'
[ERROR]         ADDED          PROPERTY   d2.10='block'
[ERROR]         ADDED          PROPERTY   d2.11='Lkotlin/Function2;'
[ERROR]         ADDED          PROPERTY   d2.12='Lkotlinx/coroutines/CoroutineScope;'
[ERROR]         ADDED          PROPERTY   d2.13='Lkotlin/coroutines/Continuation;'
[ERROR]         ADDED          PROPERTY   d2.14=''
[ERROR]         ADDED          PROPERTY   d2.15='Lkotlin/ExtensionFunctionType;'
[ERROR]         ADDED          PROPERTY   d2.16='(Ljava/util/Map;Ljava/util/Collection;Lkotlin/jvm/functions/Function2;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;'
[ERROR]         ADDED          PROPERTY   d2.17='withLoggingContext'
[ERROR]         ADDED          PROPERTY   d2.18='log4j-api-kotlin'
[ERROR]         ADDED          PROPERTY   d2.1='Lorg/apache/logging/log4j/kotlin/CoroutineThreadContext;'
[ERROR]         ADDED          PROPERTY   d2.2='map'
[ERROR]         ADDED          PROPERTY   d2.3=''
[ERROR]         ADDED          PROPERTY   d2.4=''
[ERROR]         ADDED          PROPERTY   d2.5='stack'
[ERROR]         ADDED          PROPERTY   d2.6=''
[ERROR]         ADDED          PROPERTY   d2.7='loggingContext'
[ERROR]         ADDED          PROPERTY   d2.8='withAdditionalLoggingContext'
[ERROR]         ADDED          PROPERTY   d2.9='R'
[ERROR]         ADDED          PROPERTY   k=2
[ERROR]         ADDED          PROPERTY   mv.0=1
[ERROR]         ADDED          PROPERTY   mv.1=6
[ERROR]         ADDED          PROPERTY   mv.2=0
[ERROR]         ADDED          PROPERTY   xi=48
[ERROR]     MINOR              CLASS      org.apache.logging.log4j.kotlin.ThreadContextData
[ERROR]       ADDED            METHOD     plus(org.apache.logging.log4j.kotlin.ThreadContextData)
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.NotNull
[ERROR]         ADDED          ANNOTATED  org.jetbrains.annotations.NotNull
[ERROR]         ADDED          RETURN     org.apache.logging.log4j.kotlin.ThreadContextData
[ERROR]       CHANGED          ANNOTATED  kotlin.Metadata
[ERROR]         REMOVED        PROPERTY   d1.0='...'
[ERROR]         ADDED          PROPERTY   d1.0='...'
[ERROR]         ADDED          PROPERTY   d2.20='plus'
[ERROR]         REMOVED        PROPERTY   d2.20='toString'
[ERROR]         ADDED          PROPERTY   d2.21='data'
[ERROR]         REMOVED        PROPERTY   d2.21='log4j-api-kotlin'
[ERROR]         ADDED          PROPERTY   d2.22='toString'
[ERROR]         ADDED          PROPERTY   d2.23='log4j-api-kotlin'
[ERROR]     REMOVED            VERSION    1.4.0
[ERROR]     ADDED              VERSION    1.4.1

This change requires a minor version bump to 1.5.0.

rocketraman commented 5 months ago

You need to look at the end of the BND report:

@ppkarwasz Thank you, my brain was confused between MAJOR/MINOR so I was looking at the wrong place in the report. Have pushed a commit to bump to 1.5.0-SNAPSHOT.

vy commented 4 months ago

@rocketraman, I would appreciate it if you don't force-push. This renders all historical conversations detached, since associated commits don't exist anymore.

rocketraman commented 4 months ago

@rocketraman, I would appreciate it if you don't force-push. This renders all historical conversations detached, since associated commits don't exist anymore.

@vy Yeah, GitHub is quite poor at managing code reviews with force pushes, unfortunately. My preference is generally to sacrifice the GitHub PR cleanliness in favor of a clean commit history, as the former is transient and the latter is forever. However, I recognize this is personal preference and most people tend to prefer keeping the PR clean and not caring too much about the commit log. Its unfortunate that GitHub isn't better at this, given that even git.git upstream prefers the force push / clean commit history approach.

vy commented 4 months ago

My preference is generally to sacrifice the GitHub PR cleanliness in favor of a clean commit history, as the former is transient and the latter is forever.

@rocketraman, we (almost) always use Squash and Commit in Log4j. You could have done the same for this PR. That is, your PR would appear as a single commit in main. Doesn't that address your concern?

ppkarwasz commented 4 months ago

@rocketraman, we (almost) always use Squash and Commit in Log4j. You could have done the same for this PR. That is, your PR would appear as a single commit in main. Doesn't that address your concern?

That is really a question of commit style. Personally I prefer force pushes like @rocketraman and I almost never make merge commits.

Unless the number of commits is high or commits have no well-defined purposes (e.g. a contributor changes his contribution back and forth) I don't squash either.