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

Extend the length of OpenSearchJsonLayout logger from 10k to 100k #6569

Merged
merged 10 commits into from
Mar 9, 2023

Conversation

djboris9
Copy link
Contributor

@djboris9 djboris9 commented Mar 7, 2023

Description

Especially audit logs from OpenSearch can be longer than 10k characters. If Log4J is configured with the OPENSEARCHJsonLayout pattern type, the inner message (which is a json) tends to get truncated and therefore the deserialization of the inner message fails and leads to missing information. Depending on the logs, this is not what a customer would expect to happen.

Therefore I propose to extend the truncation from 10k characters to at least 100k characters but I'm also happy if it's extended further. This allows for longer log messages and therefore should serialize much more logs correctly but potentially increases the log file growth in cases, where the messages were regularely truncated.

Example log4j2.properties:

appender.audit_rolling.type = RollingFile
appender.audit_rolling.name = audit_rolling
appender.audit_rolling.fileName = ${sys:opensearch.logs.base_path}${sys:file.separator}${sys:opensearch.logs.cluster_name}_audit.json
appender.audit_rolling.layout.type = OPENSEARCHJsonLayout
appender.audit_rolling.layout.type_name = audit
appender.audit_rolling.layout.opensearchmessagefields=x-opaque-id

Example of truncated logs, anonymized and shortened:

{
  "type": "audit",
  "timestamp": "2023-03-07T01:01:01,000+00:00",
  "level": "INFO",
  "component": "audit",
  "cluster.name": "MASKED",
  "node.name": "MASKED_DATA",
  "message": "{\"audit_trace_task_parent_id\":\"MASKED\",\"audit_cluster_name\":\"MASKED\",....\".kibana_20353",  <-- This end is truncated
  "cluster.uuid": "MASKED",
  "node.id": "MASKED"
}

Issues Resolved

No issue was created

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

Gradle Check (Jenkins) Run Completed with:

@djboris9 djboris9 marked this pull request as draft March 7, 2023 17:35
@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2023

Gradle Check (Jenkins) Run Completed with:

@djboris9 djboris9 marked this pull request as ready for review March 8, 2023 07:50
@codecov-commenter
Copy link

Codecov Report

Merging #6569 (1c46022) into main (8e0edee) will increase coverage by 0.14%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6569      +/-   ##
============================================
+ Coverage     70.65%   70.80%   +0.14%     
- Complexity    59075    59204     +129     
============================================
  Files          4804     4804              
  Lines        283075   283075              
  Branches      40806    40806              
============================================
+ Hits         200013   200436     +423     
+ Misses        66683    66160     -523     
- Partials      16379    16479     +100     
Impacted Files Coverage Δ
...pensearch/common/logging/OpenSearchJsonLayout.java 0.00% <0.00%> (ø)
...main/java/org/opensearch/node/NodeMocksPlugin.java 0.00% <0.00%> (-100.00%) ⬇️
.../index/shard/IndexShardNotRecoveringException.java 0.00% <0.00%> (-50.00%) ⬇️
.../org/opensearch/client/indices/AnalyzeRequest.java 31.00% <0.00%> (-45.00%) ⬇️
...opensearch/persistent/PersistentTasksExecutor.java 22.22% <0.00%> (-44.45%) ⬇️
...ava/org/opensearch/threadpool/ThreadPoolStats.java 52.00% <0.00%> (-29.34%) ⬇️
...a/org/opensearch/index/mapper/MapperException.java 75.00% <0.00%> (-25.00%) ⬇️
...va/org/opensearch/monitor/process/ProcessInfo.java 68.00% <0.00%> (-24.00%) ⬇️
...nsearch/index/shard/IndexingOperationListener.java 63.93% <0.00%> (-22.96%) ⬇️
...pensearch/index/mapper/MapperParsingException.java 77.77% <0.00%> (-22.23%) ⬇️
... and 500 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

if (Strings.isEmpty(type)) {
throw new IllegalArgumentException("layout parameter 'type_name' cannot be empty");
}

String messageFormat = "%m";
if (maxMessageLength < 0) {
Copy link
Collaborator

@reta reta Mar 9, 2023

Choose a reason for hiding this comment

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

👍 , do you think the upper limit would also make sense to let say 500k? (going with 2g messages is too much I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about to enforce a sane upper limit. But in the end the user still has the ability to set the maxmessagelength = 500000. On the other side, I can imagine that the integrity/completeness of audit logs are for many customers important and therefore some hard upper limit isn't desirable. This way I went for a explicit unrestricted limit you can chose with maxmessagelength = 0 and keeping the default at 10'000 chars

CHANGELOG.md Outdated
@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Support for HTTP/2 (server-side) ([#3847](https://github.com/opensearch-project/OpenSearch/pull/3847))
- Add getter for path field in NestedQueryBuilder ([#4636](https://github.com/opensearch-project/OpenSearch/pull/4636))
- Allow mmap to use new JDK-19 preview APIs in Apache Lucene 9.4+ ([#5151](https://github.com/opensearch-project/OpenSearch/pull/5151))
- The truncation limit of the OpenSearchJsonLayout logger is now configurable ([#6569](https://github.com/opensearch-project/OpenSearch/pull/6569))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please move the changelog entry to ## [Unreleased 2.x], we'll backport it to 2.x branch as well, thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with 227fdfc

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationDuringQueryPhase

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

Gradle Check (Jenkins) Run Completed with:

@djboris9
Copy link
Contributor Author

djboris9 commented Mar 9, 2023

@reta Should I squash my commits before it hopefully gets merged?

@reta
Copy link
Collaborator

reta commented Mar 9, 2023

@reta Should I squash my commits before it hopefully gets merged?

Up to you, the change is small enough (and it will be squashed on merge)

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.repositories.azure.AzureBlobContainerRetriesTests.testReadNonexistentBlobThrowsNoSuchFileException

@reta
Copy link
Collaborator

reta commented Mar 9, 2023

@andrross any concerns regarding this change? thank you

@andrross andrross added the backport 2.x Backport to 2.x branch label Mar 9, 2023
@andrross andrross merged commit 13d336c into opensearch-project:main Mar 9, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 9, 2023
* Extend the length of OpenSearchJsonLayout logger from 10 to 100k

Signed-off-by: Boris Djurdjevic <[email protected]>

* Add changelog entry for OpenSearchJsonLayout change

Signed-off-by: Boris Djurdjevic <[email protected]>

* Changelog: Add link to #6569

Signed-off-by: Boris Djurdjevic <[email protected]>

* Adapt OpenSearchJsonLayoutTests for #6569

Signed-off-by: Boris Djurdjevic <[email protected]>

* implement configurable truncation limit in OpenSearchJsonLayout logger

Signed-off-by: Boris Djurdjevic <[email protected]>

* changelog: Move #6569 PR from changes to additions

Signed-off-by: Boris Djurdjevic <[email protected]>

* move PR #6569 changelog entry to Unreleased 2.x

Signed-off-by: Boris Djurdjevic <[email protected]>

---------

Signed-off-by: Boris Djurdjevic <[email protected]>
(cherry picked from commit 13d336c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kotwanikunal pushed a commit that referenced this pull request Mar 9, 2023
#6605)

* Extend the length of OpenSearchJsonLayout logger from 10 to 100k



* Add changelog entry for OpenSearchJsonLayout change



* Changelog: Add link to #6569



* Adapt OpenSearchJsonLayoutTests for #6569



* implement configurable truncation limit in OpenSearchJsonLayout logger



* changelog: Move #6569 PR from changes to additions



* move PR #6569 changelog entry to Unreleased 2.x



---------


(cherry picked from commit 13d336c)

Signed-off-by: Boris Djurdjevic <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
mingshl pushed a commit to mingshl/OpenSearch-Mingshl that referenced this pull request Mar 24, 2023
…earch-project#6569)

* Extend the length of OpenSearchJsonLayout logger from 10 to 100k

Signed-off-by: Boris Djurdjevic <[email protected]>

* Add changelog entry for OpenSearchJsonLayout change

Signed-off-by: Boris Djurdjevic <[email protected]>

* Changelog: Add link to opensearch-project#6569

Signed-off-by: Boris Djurdjevic <[email protected]>

* Adapt OpenSearchJsonLayoutTests for opensearch-project#6569

Signed-off-by: Boris Djurdjevic <[email protected]>

* implement configurable truncation limit in OpenSearchJsonLayout logger

Signed-off-by: Boris Djurdjevic <[email protected]>

* changelog: Move opensearch-project#6569 PR from changes to additions

Signed-off-by: Boris Djurdjevic <[email protected]>

* move PR opensearch-project#6569 changelog entry to Unreleased 2.x

Signed-off-by: Boris Djurdjevic <[email protected]>

---------

Signed-off-by: Boris Djurdjevic <[email protected]>
Signed-off-by: Mingshi Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants