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

Use sanitized message in logs #25433

Merged
merged 11 commits into from
Nov 24, 2021
Merged

Use sanitized message in logs #25433

merged 11 commits into from
Nov 24, 2021

Conversation

lmolkova
Copy link
Member

spin-off from #25247

This PR

  • fixes unsanitized (with newline) messages in logs
  • adds a tiny perf improvement for it to avoid using regex for new line replacement

Benchmark

Benchmark                           Mode  Cnt    Score    Error  Units
MyBenchmark.customReplaceNewLine    avgt    6  433.248 ▒ 12.022  ns/op
MyBenchmark.replaceAllNewLine       avgt    6  731.998 ▒ 13.366  ns/op

MyBenchmark.customReplaceNoNewLine  avgt    6   11.642 ▒  0.140  ns/op
MyBenchmark.replaceAllNoNewLine     avgt    6   54.876 ▒  2.575  ns/op
 @Benchmark
  public void replaceAllNoNewLine(Blackhole bh) {
      bh.consume(NEW_LINE_PATTERN.matcher(logNoNewLine).replaceAll(""));
  }

  @Benchmark
  public void replaceAllNewLine(Blackhole bh) {
      bh.consume(NEW_LINE_PATTERN.matcher(logWithNewLine).replaceAll(""));
  }

  @Benchmark
  public void customReplaceNewLine(Blackhole bh) {
      bh.consume(sanitizeLogMessageInput(logWithNewLine));
  }

  @Benchmark
  public void customReplaceNoNewLine(Blackhole bh) {
      bh.consume(sanitizeLogMessageInput(logNoNewLine));
  }

lmolkova and others added 3 commits November 24, 2021 11:38
…n/logging/LoggingUtils.java

Co-authored-by: Alan Zimmer <48699787+alzimmermsft@users.noreply.github.com>
@lmolkova lmolkova enabled auto-merge (squash) November 24, 2021 19:59

@Test
public void withCL() {
assertEquals("\thelloworld", LoggingUtils.sanitizeLogMessageInput("\r\thello\r" + NEW_LINE + "world"));
Copy link
Contributor

@kasobol-msft kasobol-msft Nov 24, 2021

Choose a reason for hiding this comment

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

I wonder if the expected value should be "t\hello world". i.e. should we replace \n or \r or \r\n with white space?
If somebody is processing logs in some sophisticated way (i.e. attempts to do NLP) "helloworld" and "hello world" may make a difference.
(this might be out of scope of this PR).

@lmolkova lmolkova merged commit bdc5fb5 into Azure:main Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core azure-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants