Skip to content

Conversation

andreycha
Copy link

@andreycha andreycha commented Apr 26, 2023

Change
Add Peek() method to ThreadContextStack and LogicalThreadContextStack to be able to get the most recent value from the context without removing it.

Motivation
In the scope of ElasticCommonSchema .NET integration for log4net log entry properties are being read and written to ECS format. In case of context stacks current implementation takes string representation of the stack. It works fine when stack (i.e., property) has a single value, but there are also cases when stack contains multiple values (same or different ones). With the proposed changes it will be able to take the most recent value, in the context of which the current log entry is being written.

Consider following scenario:

using (LogicalThreadContext.Stacks["Id"].Push("123"))
{
    log.Info("Log 1"); // currently has "123" in the log metadata

    using (LogicalThreadContext.Stacks["Id"].Push("456"))
    {
        log.Info("Log 2"); // currently has "123 456" in the log metadata; should have "456" instead

        using (LogicalThreadContext.Stacks["Id"].Push("789"))
        {
            log.Info("Log 3"); // currently has "123 456 789" in the log metadata; should have "789" instead
        }
    }
}

Alternative solutions considered
An alternative approach might be to just pop the most recent value and then push it again onto the stack. Problem here is that it creates a new scope which will not be closed as no one will call Dispose() on the returned result from Push().

@andreycha
Copy link
Author

Hi @fluffynuts , could you please check this PR? Thanks!

Copy link
Contributor

@fluffynuts fluffynuts left a comment

Choose a reason for hiding this comment

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

LGTM

I can't make any promises about when this gets out live - I haven't had a lot of time to put into this in a while ): I'm currently formulating a plan to do beta / prerelease packages with less release friction so that perhaps at least things can get out quicker, but I have to speak with more venerable Apache people on what's process would be acceptable.

@fluffynuts fluffynuts merged commit 22d3378 into apache:master May 3, 2023
@andreycha
Copy link
Author

Thanks! Is there a way to somehow get notified when a new release comes out?

@andreycha andreycha deleted the feat/support-peek-in-stacks branch May 3, 2023 13:10
@fluffynuts
Copy link
Contributor

@andreycha I guess you could sub to apache logging email lists; but I really want to figure out a way to do beta releases, and I doubt they'd go through that, so 🤷 I haven't figured this out yet ): I have this tension between "actual releases are a major mission with a lot of dead time whilst I wait for a bunch of Java people to vote on a dotnet thing that they have no context about" and "I just want to release fixes" )':

@andreycha
Copy link
Author

Yeah, I can relate. Anyway, looking forward to the new release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants