Skip to content

Logging emitter to publish query and other metric events as valid json objects#8359

Merged
himanshug merged 7 commits intoapache:masterfrom
himanshug:json_log
Aug 27, 2019
Merged

Logging emitter to publish query and other metric events as valid json objects#8359
himanshug merged 7 commits intoapache:masterfrom
himanshug:json_log

Conversation

@himanshug
Copy link
Copy Markdown
Contributor

@himanshug himanshug commented Aug 21, 2019

Description

This patch enables pushing druid metrics in log as valid json objects which are supported out of the box by various systems used for storing and searching logs. This aids us in investigating various issues in Druid cluster.

It has following changes.

  • LoggingEmitter is modified to emit jsonMapper.writeValueAsString(event) instead of format("Event [%s]", jsonMapper.writeValueAsString(event)) .
  • QueryMetrics puts the query context into metric event as is instead of converting it to String.
  • EmittingRequestLoggerProvider uses only available DefaultRequestLogEventBuilderFactory by default when publishing request logs using the emitter.

Side note: I also drop following in my log4j.xml to send the json object published as is...

<Configuration status="WARN">
<Appenders>
 ...
  <Console name="msgonly" target="SYSTEM_OUT">
    <PatternLayout pattern="%m%n"/>
  </Console>
...
</Appenders>
<Loggers>
...
  <Logger name="org.apache.druid.java.util.emitter.core.LoggingEmitter" additivity="false" level="info">
    <AppenderRef ref="msgonly"/>
  </Logger>
...
</Loggers>
</Configuration>

This PR has:

  • been self-reviewed.
  • been tested in a Druid cluster.

Release Note Update:

LoggingEmitter is modified to emit jsonMapper.writeValueAsString(event) instead of format("Event [%s]", jsonMapper.writeValueAsString(event)) . Anyone depending on the older specifics of message printed by LoggingEmitter would need to update accordingly.

@himanshug
Copy link
Copy Markdown
Contributor Author

all builds except coverage are passing which looks like spurious failure.

private static final DefaultRequestLogEventBuilderFactory INSTANCE = new DefaultRequestLogEventBuilderFactory();

@JsonCreator
public static DefaultRequestLogEventBuilderFactory instance(String ignored)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a Javadoc comment explaining why this is needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why added this method in this PR? If this is the default factory, why would anybody want to specify druid.request.logging.requestLogEventBuilderFactory=xyz in the config?

Please clarify both Javadoc and the PR description about this. Also, please add instructions for Druid cluster operators regarding this PR (what configs should be changed, what existing things could break.) Adding something to Druid's documentation might also be needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so without this patch , I added following in my properties ....

druid.emitter=logging
druid.request.logging.type=emitter
druid.request.logging.feed=requests

that failed with following error....

1) druid.request.logging.requestLogEventBuilderFactory - may not be null
  at org.apache.druid.guice.JsonConfigProvider.bind(JsonConfigProvider.java:164) (via modules: com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> org.apache.druid.guice.QueryableModule)
  at org.apache.druid.guice.JsonConfigProvider.bind(JsonConfigProvider.java:164) (via modules: com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> org.apache.druid.guice.QueryableModule)
  while locating com.google.common.base.Supplier<org.apache.druid.server.log.RequestLoggerProvider>
  at org.apache.druid.guice.JsonConfigProvider.bind(JsonConfigProvider.java:165) (via modules: com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> org.apache.druid.guice.QueryableModule)
  while locating org.apache.druid.server.log.RequestLoggerProvider
  at org.apache.druid.guice.QueryableModule.configure(QueryableModule.java:48) (via modules: com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> org.apache.druid.guice.QueryableModule)
  while locating org.apache.druid.server.log.RequestLogger
    for the 5th parameter of org.apache.druid.server.QueryLifecycleFactory.<init>(QueryLifecycleFactory.java:52)
  at org.apache.druid.server.QueryLifecycleFactory.class(QueryLifecycleFactory.java:52)
  while locating org.apache.druid.server.QueryLifecycleFactory
    for the 1st parameter of org.apache.druid.server.QueryResource.<init>(QueryResource.java:113)
  at org.apache.druid.server.QueryResource.class(QueryResource.java:78)
  while locating org.apache.druid.server.QueryResource

as per the documentation , above was not supposed to happen. so made the change in EmittingRequestLoggerProvider to hardcode usage of DefaultRequestLogEventBuilderFactory by default .
no change is needed in the docs as things would work as expected.

for this specific change: fixing the json creation is not strictly necessary, so removed that to keep things simple in this PR.

@himanshug
Copy link
Copy Markdown
Contributor Author

also added a blurb to be added to release notes due to change in this PR . I don't think it impacts anyone but just in case.

@himanshug himanshug added this to the 0.16.0 milestone Aug 27, 2019
@himanshug
Copy link
Copy Markdown
Contributor Author

@leventov thanks for reviewing

@himanshug himanshug merged commit 4d87a19 into apache:master Aug 27, 2019
@himanshug himanshug deleted the json_log branch August 27, 2019 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants