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

Added MDC #557

Merged
merged 12 commits into from
Dec 16, 2017
Merged

Added MDC #557

merged 12 commits into from
Dec 16, 2017

Conversation

MinnDevelopment
Copy link
Member

@MinnDevelopment MinnDevelopment commented Dec 1, 2017

Pull Request Etiquette

There are several guidelines you should follow in order for your
Pull Request to be merged.

It is sometimes better to include more changes in a single commit.
If you find yourself having an overwhelming amount of commits, you
can rebase your branch.

Template

Pull Request

Using MDC we are able to categorize our logging events per shard/jda instance.
You can set a custom context map by using JDABuilder.setContextMap(ConcurrentMap<String, String>) which will be set in all JDA controlled threads.

Additionally I added a new method to the IAudioSendSystem to set the context in custom implementations. This method is declared as default due to backward compatibility but is implemented in the DefaultSendSystem.

Using logback and log4j MDC can be used to filter by shard:

Logback

public class MyFilter extends Filter<ILoggingEvent>
{
    @Override
    public FilterReply decide(ILoggingEvent event)
    {
        String shard = MDC.get("jda.shard");
        if (shard != null && shard.equals("[0 / 2]"))
            return FilterReply.DENY;
        return FilterReply.NEUTRAL;
    }
}

Log4j

public class MyFilter extends Filter
{
    @Override
    public int decide(LoggingEvent event)
    {
        String shard = MDC.get("jda.shard");
        if (shard != null && shard.equals("[0 / 2]"))
            return DENY;
        return NEUTRAL;
    }
}

contextMap.put("jda.shard.id", String.valueOf(shardInfo.getShardId()));
contextMap.put("jda.shard.total", String.valueOf(shardInfo.getShardTotal()));
}
MDC.setContextMap(contextMap);
Copy link
Contributor

Choose a reason for hiding this comment

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

This nukes any MDC values that client code might have set 😢
In your own threads I don't mind manually managing the contextMap, but this is rather destructive
Since buildAsync isn't enough, you'd have to invoke buildAsync asynchronously and explain why that's done that way and hope no one refactors it away

Copy link
Member Author

Choose a reason for hiding this comment

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

The client is able to control the map

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand your concern though. I could make use of MDC.MDCClosable and just put all the values in the existing context for the build process

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like a fair alternative

// set MDC metadata for build thread
Map<String, String> previousContext = MDC.getCopyOfContextMap();
contextMap.forEach(MDC::put);
// init code
MDC.setContextMap(previousContext);

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as it's addressed I'm fine with it 👍

@MinnDevelopment MinnDevelopment added this to the Release 3.4 milestone Dec 9, 2017
@MinnDevelopment MinnDevelopment changed the base branch from master to development December 10, 2017 13:37
# Conflicts:
#	src/main/java/net/dv8tion/jda/core/JDABuilder.java
#	src/main/java/net/dv8tion/jda/core/entities/impl/JDAImpl.java
@MinnDevelopment MinnDevelopment added status: in progress already in the process of being implemented status: completed has been completed but is not yet released and removed status: in progress already in the process of being implemented labels Dec 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: completed has been completed but is not yet released type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants