msq path to emit unparseable exceptions by TSFenwick · Pull Request #13670 · apache/druid

TSFenwick · 2023-01-13T23:20:30Z

Emit UnparseableExceptions in realtime as a new event.

Description

The goal of this PR is to emit an unparseableEvent in real time that an admin of druid might be interested in storing elsewhere. The new event shouldn't be emitted by default and should be opted into.

Release note

For tips about how to write a good release note, see Release notes.

Key changed/added classes in this PR

ServiceLogEvent
ParseExceptionHandler
MSQCompositeWarningReporter
MSQFilteredEmitterReporter

This PR has:

cryptoe

Left some comments.
Thanks for taking this up.
Please update the description whenever you have time.

...-core/multi-stage-query/src/main/java/org/apache/druid/msq/kernel/FrameProcessorFactory.java

cryptoe · 2023-01-14T01:56:11Z

...uery/src/main/java/org/apache/druid/msq/indexing/error/MSQWarningReportLimiterPublisher.java

        return;
      }
    }
+    if (CannotParseExternalDataFault.CODE.equals(errorCode)) {


This is what I would do:
Rename MSQWarningReportPublisher to ExceptionPublisher.

Create a new CompositeExceptionPublisher.

One implementation can be of a FilteredEmitterExceptionPublisher where in the constructor you take the error code.

It would be cool to avoid This call
:MSQErrorReport msqFault = MSQErrorReport.fromException(workerId, host, stageNumber, e);

:MSQErrorReport msqFault = MSQErrorReport.fromException(workerId, host, stageNumber, e);
will definitely avoid repeating this call

How do i go about attaching a debugger in MSQ to see if these reports are being captured/emitted

I'm not sure i follow

This is what I would do:
Rename MSQWarningReportPublisher to ExceptionPublisher.

Create a new CompositeExceptionPublisher.

One implementation can be of a FilteredEmitterExceptionPublisher where in the constructor you take the error code.

Are you saying create another class that implements the interface that i will rename.

The CompositeExceptionPublisher i don't understand?
FilteredEmitterExceptionPublisher i also don't understand

How do i go about attaching a debugger in MSQ to see if these reports are being captured/emitted

MSQWarningsTest#testThrowExceptionWhenParseExceptionsExceedLimit should be a good one.

CompositeExceptionPublisher will implement the renamed interface ExceptionPublisher something like

class CompositeExceptionPublisher implements ExceptionPublisher{ List<ExceptionPublisher> delegates; public CompositeExceptionPublisher(List<ExceptionPublisher> delegates){ this.delegates=delegates; } void publishException(int stageNumber, Throwable e){ for(ExceptionPublisher delegate:delegates){ delegate.publishException(stage,e); } } void close() throws IOException{ for(ExceptionPublisher delegate:delegates){ delegate.close(); } } }

Now you can create a new impl FilteredEmitterExceptionPublisher and pass it errorCodes in the constructor.

Then in WorkerImpl#runTask method create a CompositeExceptionPublisher with MSQWarningReportLimiterPublisher and FilteredEmitterExceptionPublisher and pass it along.

Maybe this strategy can work.

@cryptoe How does this look?
We have the Limiter publisher and we pass in the composite publisher that contains the normal publisher and the emitter publisher

have a couple of todos as well like setting up the correct event to emit, and getting more relevant information

...ons-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestWorkerContext.java

LakshSingla

How do i go about attaching a debugger in MSQ to see if these reports are being captured/emitted

There should be a class MSQWarningsTest which test a few conditions when warnings should be thrown. Also, to test if the metrics are emitted, a few ways would be to either create a mock emitter that captures the calls made to it or spy on the existing noop emmiter and then verifying the arguments.

LakshSingla · 2023-01-24T18:11:21Z

...ry/src/main/java/org/apache/druid/msq/indexing/error/MSQFilteredEmitterWarningPublisher.java

+import java.io.IOException;
+import java.util.Set;
+
+public class MSQFilteredEmitterWarningPublisher implements MSQWarningPublisher


I think it might be appropriate to have separate classes:

MSQFilteredPublisher which takes in a delegate and publishes the warning only if it passes through the filter

MSQEmitterPublisher that emits the warning if publishException is called

That makes sense, but this is the only consumer of filtering and emitting right now so does that make sense to do that level of abstraction?

If this is necessary i'll leave this change to later as it is pretty straightforward

Works for me 👍

...ry/src/main/java/org/apache/druid/msq/indexing/error/MSQFilteredEmitterWarningPublisher.java

LakshSingla · 2023-01-24T18:13:11Z

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/WorkerContext.java


  Bouncer processorBouncer();
+
+  Emitter emitter();


Can the emitter be nullable? If so, appropriate checks and annotations would be helpful

We are getting this from TaskToolBox and the emitter there isn't labeled as nullable

core/src/main/java/org/apache/druid/java/util/emitter/service/ServiceLogEvent.java

TSFenwick · 2023-01-28T01:36:32Z

...ry/src/main/java/org/apache/druid/msq/indexing/error/MSQFilteredEmitterWarningPublisher.java

+  public MSQFilteredEmitterWarningPublisher(
+      final String workerId,
+      final String controllerTaskId,
+      final String taskId,


add datasource

...uery/src/main/java/org/apache/druid/msq/indexing/error/MSQWarningReportLimiterPublisher.java

TSFenwick · 2023-01-28T01:38:27Z

processing/src/test/java/org/apache/druid/segment/incremental/ParseExceptionHandlerTest.java

        Integer.MAX_VALUE,
-        0
+        0,
+        null


change nulls to a noop parse exception handler if possible

TSFenwick · 2023-01-30T21:02:13Z

...uery/src/main/java/org/apache/druid/msq/indexing/error/MSQWarningReportLimiterPublisher.java

 * is trully published
 */
-public class MSQWarningReportLimiterPublisher implements MSQWarningReportPublisher
+public class MSQWarningReportLimiterPublisher implements MSQWarningPublisher


change class to MSQWarningLimiterPublisher

server/src/main/java/org/apache/druid/segment/realtime/plumber/RealtimePlumberSchool.java

TSFenwick · 2023-01-30T21:03:35Z

indexing-service/src/main/java/org/apache/druid/indexing/common/TaskMetadata.java

@@ -0,0 +1,38 @@
+package org.apache.druid.indexing.common;


is this the correct package for this?

This class seems avoidable. Looks like this is introduced cause of parseExceptionHandler cause you need the taskId, groupId, data source.
TaskID, groupId is part of the Task.java and seems to be repeated here.

What happens if task is operating on 2 data sources ?

So for the above reasons I think we should think of another way.

Can a task operate on two datasources?

ok i understand that it can now after chatting with some other people. Would it be clearer if i said TargetDataSource?

Something like parseExceptionMetadata ?

TSFenwick · 2023-01-31T00:48:14Z

processing/src/test/java/org/apache/druid/segment/incremental/ParseExceptionHandlerTest.java

-        0
+        0,
+        null
    );


missing task metadata

suneet-s

High level design feedback:

I think a new ErrorLogger and set of associated classes should be added. Similar to RequestLogger, they can be backed by different implementations like logging to a file or emitting somewhere.
ParseExceptionHandler should accept an ErrorLogger in it's constructor. Any class that instantiates a ParseExceptionHandler should be able to get ErrorLogger from the injector
Update ParseExceptionHandler#handle to accept a Map<String, Object> metadata in addition to the parseException. This makes it clear that the caller is responsible for passing additional metadata about the exception. If the metadata is consistent, maybe the method should accept a POJO instead of a map.

If this approach works, it would probably be best to split this PR into 2 - one where you introduce the ErrorLogger and another where it is wired up to the ParseExceptionHandler

cryptoe

I agree with suneet on the 1) and 2) points. That would be much cleaner. Regarding the third point since we will not have common metadata like taskId where ever the handle method is called, it may be better to just wire the common dimensions it in the constructor.

cryptoe · 2023-02-02T06:47:24Z

indexing-service/src/main/java/org/apache/druid/indexing/common/TaskMetadata.java

@@ -0,0 +1,38 @@
+package org.apache.druid.indexing.common;


Something like parseExceptionMetadata ?

…vent was overeager with module as i followed RequestLogger and QueryableModule for inspiration. this should be scoped down more also cleaned up checkstyle issues

TSFenwick · 2023-03-30T19:04:31Z

@suneet-s i'll go down the path of creating error logger, but i think the name will need to be changed.

github-actions · 2024-02-01T00:17:11Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

github-actions · 2024-03-01T00:17:39Z

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

msq path to emit unparseable exceptions

fe20947

cryptoe reviewed Jan 14, 2023

View reviewed changes

clintropolis added Area - Metrics/Event Emitting Area - Operations Ease of Use labels Jan 18, 2023

remove unused import

dbc321a

cryptoe added the Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 label Jan 19, 2023

basic structure of emitting, might need to change order of dependencies.

ddf6100

have a couple of todos as well like setting up the correct event to emit, and getting more relevant information

TSFenwick commented Jan 23, 2023

View reviewed changes

...ons-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestWorkerContext.java Outdated Show resolved Hide resolved

better ordering for msq warning publisher

f911860

LakshSingla reviewed Jan 24, 2023

View reviewed changes

TSFenwick added 2 commits January 24, 2023 19:18

emit real events with relevant data

a42d23a

emit parse exceptions from all druid

697abc9

TSFenwick commented Jan 28, 2023

View reviewed changes

core/src/main/java/org/apache/druid/java/util/emitter/service/ServiceLogEvent.java Show resolved Hide resolved

TSFenwick commented Jan 28, 2023

View reviewed changes

...uery/src/main/java/org/apache/druid/msq/indexing/error/MSQWarningReportLimiterPublisher.java Show resolved Hide resolved

TSFenwick commented Jan 28, 2023

View reviewed changes

TSFenwick commented Jan 30, 2023

View reviewed changes

server/src/main/java/org/apache/druid/segment/realtime/plumber/RealtimePlumberSchool.java Show resolved Hide resolved

TSFenwick commented Jan 30, 2023

View reviewed changes

TSFenwick commented Jan 31, 2023

View reviewed changes

add javadocs, clean up whitespaces and rename class

b3a44bf

suneet-s reviewed Feb 1, 2023

View reviewed changes

cryptoe reviewed Feb 2, 2023

View reviewed changes

TSFenwick added 2 commits February 3, 2023 13:08

create ServiceEventLogger, to make optional emission of service log e…

dec78b0

…vent was overeager with module as i followed RequestLogger and QueryableModule for inspiration. this should be scoped down more also cleaned up checkstyle issues

rename TaskMetadata to be more narrowly scoped to ParseExceptionMetadata

2081695

github-actions bot added the stale label Feb 1, 2024

github-actions bot closed this Mar 1, 2024

-
+,
+                      null
                   );

Conversation

TSFenwick commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release note

Key changed/added classes in this PR

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TSFenwick Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LakshSingla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TSFenwick Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suneet-s left a comment

Choose a reason for hiding this comment

Uh oh!

cryptoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TSFenwick commented Mar 30, 2023

Uh oh!

github-actions bot commented Feb 1, 2024

Uh oh!

github-actions bot commented Mar 1, 2024

Uh oh!

Reviewers

TSFenwick commented Jan 13, 2023 •

edited

Loading

TSFenwick Jan 17, 2023 •

edited

Loading

TSFenwick Jan 24, 2023 •

edited

Loading