Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Peek() method to ThreadContextStack and LogicalThreadContextStack. #101

Merged
merged 3 commits into from
May 3, 2023
Merged

Add Peek() method to ThreadContextStack and LogicalThreadContextStack. #101

merged 3 commits into from
May 3, 2023

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.

3 participants