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 layout #145

Closed
Closed

Conversation

dgempiuc
Copy link
Contributor

layout field added and encode method modified by using the layout while adding the messages to the log file.

@cla-checker-service
Copy link

cla-checker-service bot commented Sep 30, 2021

💚 CLA has been signed

@apmmachine
Copy link
Contributor

apmmachine commented Sep 30, 2021

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2022-08-21T10:31:22.417+0000

  • Duration: 3 min 0 sec

Steps errors 2

Expand to view the steps failures

Load a resource file from a library
  • Took 0 min 0 sec . View more details here
  • Description: approval-list/elastic/ecs-logging-java.yml
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: githubApiCall: The REST API call https://api.github.com/orgs/elastic/members/dgempiuc return the message : java.lang.Exception: httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/dgempiuc : httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/dgempiuc : Code: 404Error: {"message":"User does not exist or is not a member of the organization","documentation_url":"https://docs.github.com/rest/reference/orgs#check-organization-membership-for-a-user"}

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@fapena-galileo
Copy link

Any news on this PR? It would be very useful to be able to configure the layout to customize the message.

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

@dgempiuc this looks like a reasonable functionality addition, thanks.
Note that we won't be able to merge the PR without you signing the Contributor Agreement with your GitHub email.

@felixbarny any reason you see for not adding it?

@@ -54,6 +55,7 @@ public class EcsEncoder extends EncoderBase<ILoggingEvent> {
private boolean includeOrigin;
private final List<AdditionalField> additionalFields = new ArrayList<AdditionalField>();
private OutputStream os;
protected Layout<ILoggingEvent> layout;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
protected Layout<ILoggingEvent> layout;
protected Layout<ILoggingEvent> messageLayout;

if(layout == null) {
EcsJsonSerializer.serializeFormattedMessage(builder, event.getFormattedMessage());
} else {
EcsJsonSerializer.serializeFormattedMessage(builder, this.layout.doLayout(event));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EcsJsonSerializer.serializeFormattedMessage(builder, this.layout.doLayout(event));
EcsJsonSerializer.serializeFormattedMessage(builder, this.messageLayout.doLayout(event));

Comment on lines +211 to +214
public Layout<ILoggingEvent> getLayout() {
return this.layout;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public Layout<ILoggingEvent> getLayout() {
return this.layout;
}

No need for a getter

Comment on lines +215 to +217
public void setLayout(Layout<ILoggingEvent> layout) {
this.layout = layout;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public void setLayout(Layout<ILoggingEvent> layout) {
this.layout = layout;
}
public void setMessageLayout(Layout<ILoggingEvent> messageLayout) {
this.messageLayout = messageLayout;
}

Add a javadoc saying this Layout will be applied specifically to format the message field based on the logging event.

@felixbarny
Copy link
Member

@felixbarny any reason you see for not adding it?

No objections but it would be interesting to know about the use case this solves. Why would you want to use a layout within the message field?

@rdifrango
Copy link
Contributor

@felixbarny the reason for needing this is covered by #144, basically being able to mask fields in environments where you are handling sensitive data would be good to have in this encoder.

If @dgempiuc can't update this PR to get it merged, I would be willing to contribute this as well.

@felixbarny felixbarny linked an issue Sep 9, 2022 that may be closed by this pull request
@felixbarny
Copy link
Member

If @dgempiuc can't update this PR to get it merged, I would be willing to contribute this as well.

SGTM. But please be sure to take his commits as the foundation so that he still appears as a co-author of your PR.

@maxiking445
Copy link

@felixbarny the reason for needing this is covered by #144, basically being able to mask fields in environments where you are handling sensitive data would be good to have in this encoder.

If @dgempiuc can't update this PR to get it merged, I would be willing to contribute this as well.

Any news here? @rdifrango

@rdifrango
Copy link
Contributor

rdifrango commented Apr 18, 2023

@maxiking445 Unfortunately, I haven't had time to get back to it...I had it staged and started the proposed changes but haven't yet finished them :(

@rdifrango
Copy link
Contributor

@maxiking445 I'm working through the process now to contribute this back. It'll likely be in the form of a new PR with the commits from @dgempiuc as the base.

@rdifrango
Copy link
Contributor

Replaced by #220

@rdifrango
Copy link
Contributor

@JonasKunz now that #220 is merged we should go ahead and close this one as it supersedes it.

@JonasKunz JonasKunz closed this Jan 17, 2024
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.

Adding Layout to logback-ecs-encoder
8 participants