#618 - Add ability to name specific wiremock instance #2448

kapishmalik · 2023-10-16T16:48:42Z

Added ability to name specific wiremock instance.

References

Submitter checklist

Recommended: Join WireMock Slack to get any help in #help-contributing or a project-specific channel like #wiremock-java
Recommended: If you participate in Hacktoberfest 2023, make sure you're signed up there and in the WireMock form
The PR request is well described and justified, including the body and the references
The PR title represents the desired changelog entry
The repository's code style is followed (see the contributing guide)
Test coverage that demonstrates that the change works as expected
For new features, there's necessary documentation in this pull request or in a subsequent PR to [wiremock.org]

tomakehurst · 2023-10-16T17:24:24Z

This is looking good so far but let's add add a test that checks the name is actually emitted in logs.

Also, what might be interesting is to also have it affect the Slf4jNotifier that's used when starting programmatically. Specifically, we could modify line 23 to make it an instance field and set the logger name to the server name, something like:

private final Logger log = LoggerFactory.getLogger(firstNonNull(serverName, "WireMock"));

pks-1981 · 2023-10-17T05:00:18Z

src/main/java/com/github/tomakehurst/wiremock/common/Slf4jNotifier.java

@@ -15,19 +15,25 @@
 */
 package com.github.tomakehurst.wiremock.common;

+import static org.apache.commons.lang3.ObjectUtils.firstNonNull;


Please use ParameterUtils#getFirstNonNull

Incorporated

tomakehurst · 2023-10-17T08:13:41Z

src/main/java/com/github/tomakehurst/wiremock/common/Slf4jNotifier.java


  private final boolean verbose;

  public Slf4jNotifier(boolean verbose) {
    this.verbose = verbose;
  }

+  public void updateLogger(final String serverName) {
+    log = LoggerFactory.getLogger(getFirstNonNull(serverName, "WireMock"));


Suggest we do this in a constructor overload rather than mutating

TBH I am not site about making the logger non-static for that purpose. Maybe there is API would could invoke against a static field? Concurrency is not a problem here

done... incorporated.

oleg-nenashev

Looks good overall, requesting changes because of unverified lass cast, and I second @tomakehurst's note about the APIs though do not feel strongly about that

oleg-nenashev · 2023-10-17T08:26:24Z

src/main/java/com/github/tomakehurst/wiremock/common/Slf4jNotifier.java


  private final boolean verbose;

  public Slf4jNotifier(boolean verbose) {
    this.verbose = verbose;
  }

+  public void updateLogger(final String serverName) {
+    log = LoggerFactory.getLogger(getFirstNonNull(serverName, "WireMock"));


TBH I am not site about making the logger non-static for that purpose. Maybe there is API would could invoke against a static field? Concurrency is not a problem here

oleg-nenashev · 2023-10-17T08:27:53Z

src/main/java/com/github/tomakehurst/wiremock/core/WireMockConfiguration.java

+  public WireMockConfiguration serverName(String serverName) {
+
+    this.serverName = serverName;
+    ((Slf4jNotifier) notifier).updateLogger(serverName);


This unchecked cast impacts extensibility IMO. I suggest doing proper instanceof verification

…-to-name-specific-wiremock-instance

oleg-nenashev

Options#getServerName() is still a breaking change, because there is no default implementation
https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html

kapishmalik · 2023-10-26T10:45:31Z

Options#getServerName() is still a breaking change, because there is no default implementation https://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html

incorporated

oleg-nenashev

My comments are addressed

tomakehurst · 2023-11-02T18:20:33Z

src/test/java/com/github/tomakehurst/wiremock/ServerNameInLogsAcceptanceTest.java

+  public void checkServerNameIsPrintedInLogs() {
+
+    ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
+    System.setOut(new PrintStream(byteArrayOutputStream));


We need to un-set this in an @AfterEach block, otherwise it might suppress the output from other tests when running everything.

tomakehurst · 2023-11-02T18:20:58Z

src/main/java/com/github/tomakehurst/wiremock/common/Slf4jNotifier.java

    this.verbose = verbose;
  }

+  public void setLogger(String loggerName) {


I still think we should try to get rid of this and make the logger final.

Resolves wiremock#618 - Add ability to name specific wiremock instance

c075444

kapishmalik requested a review from a team as a code owner October 16, 2023 16:48

kapishmalik changed the title ~~Resolves #618 - Add ability to name specific wiremock instance~~ #618 - Add ability to name specific wiremock instance Oct 16, 2023

kapishmalik added 2 commits October 16, 2023 23:52

review comment changes

b1b3795

small refactoring

16a55c9

pks-1981 reviewed Oct 17, 2023

View reviewed changes

review comment change

ecd0b3a

kapishmalik requested a review from pks-1981 October 17, 2023 05:24

pks-1981 approved these changes Oct 17, 2023

View reviewed changes

tomakehurst reviewed Oct 17, 2023

View reviewed changes

oleg-nenashev added enhancement hacktoberfest-accepted labels Oct 17, 2023

oleg-nenashev requested changes Oct 17, 2023

View reviewed changes

review comment changes

334b1cf

kapishmalik requested review from tomakehurst, pks-1981 and oleg-nenashev October 17, 2023 09:23

kapishmalik added 2 commits October 17, 2023 15:47

fix UT

c3238b1

Merge remote-tracking branch 'origin/master' into feature/add-ability…

a15a4f5

…-to-name-specific-wiremock-instance

pks-1981 approved these changes Oct 17, 2023

View reviewed changes

oleg-nenashev requested changes Oct 25, 2023

View reviewed changes

add default method for returning server name

5eb81e1

kapishmalik requested a review from oleg-nenashev October 26, 2023 10:45

oleg-nenashev approved these changes Oct 28, 2023

View reviewed changes

tomakehurst reviewed Nov 2, 2023

View reviewed changes

oleg-nenashev added the needs-tom Tom's Train Project :) label Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#618 - Add ability to name specific wiremock instance #2448

#618 - Add ability to name specific wiremock instance #2448

kapishmalik commented Oct 16, 2023

tomakehurst commented Oct 16, 2023

pks-1981 Oct 17, 2023

kapishmalik Oct 17, 2023

tomakehurst Oct 17, 2023

oleg-nenashev Oct 17, 2023

kapishmalik Oct 17, 2023

oleg-nenashev left a comment

oleg-nenashev Oct 17, 2023

oleg-nenashev Oct 17, 2023

kapishmalik Oct 17, 2023

oleg-nenashev left a comment •

edited

kapishmalik commented Oct 26, 2023

oleg-nenashev left a comment

tomakehurst Nov 2, 2023

tomakehurst Nov 2, 2023

#618 - Add ability to name specific wiremock instance #2448

Are you sure you want to change the base?

#618 - Add ability to name specific wiremock instance #2448

Conversation

kapishmalik commented Oct 16, 2023

References

Submitter checklist

tomakehurst commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment • edited

Choose a reason for hiding this comment

kapishmalik commented Oct 26, 2023

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment •

edited