Add datadog tracking to record schema validation errors #13393

alovew · 2022-06-01T20:50:19Z

Add Datadog tracking for record schema validation errors

alovew · 2022-06-08T18:26:08Z

@lmossman this correctly recorded a record schema validation error in a test run on dev, and it's showing up in Datadog: https://app.datadoghq.com/metric/explorer?start=1654709117831&end=1654712717831&paused=false#N4IgNghgng9grgFxALlADxQRgDQil3AdwEsATBACwJAoFNiBzCpZTAX11NoDNiA7YgmIw+KUAigAHWihBCAtrQDOtAE7FlIXKtoBHOMoRKUAbVA6lkkSoD63GKvkQWc4opXrNufWo3HkZiB8EIqyPqpQmFogpM4QNkrwqgDGMsggigjqycbeBhGySnDyyIQOANZqAHQ6yQ6kCcl0TjYAbhBgZM7CfDZqqg7AAFRsVRBKNnVwfAgAFACUIBygwaHp4VAATNGxCPGJcClpGbRZxDnRG7IQrQylFdW19Y3N8e2duz19qgOqw6PjSbwGYLJYAXVw9kccEg-kCUPkMIgYXykSW2FACKRKLUW3BuCUEjAaVAkg6pwQx1IMAYk1hSnO0U6fFoNgk0kKMA+TP4rJI5Co6T4DicYHRMWIlkgUDZUmOACMIKpjGwwWw2EA

lmossman

Left some comments, main one about refactoring how the datadog-specific inputs are passed into this class to be more explicit, encapsulated, and less prone to NullPointerExceptions. Happy to discuss more over zoom/slack if you want!

lmossman · 2022-06-08T23:52:41Z

airbyte-metrics/metrics-lib/src/main/java/io/airbyte/metrics/lib/OssMetricsRegistry.java

+  NUM_RECORD_SCHEMA_VALIDATION_ERRORS(MetricEmittingApps.WORKER,
+      "record_schema_validation_error",
+      "number of record schema validation errors");


This won't actually be tracking the number of errors for a given stream, right? Because we are just emitting 1 for each stream that had any errors, and we are capping the number of validations we perform for a stream once we reach a certain number?

Maybe this should be called something like SOURCE_STREAMS_WITH_RECORD_SCHEMA_VALIDATION_ERRORS? Open to other suggestions on naming here

I think I was trying to use NUM because it is a count metric, but maybe NUM_SOURCE_STREAMS_WITH_RECORD_SCHEMA_VALIDATION_ERRORS

lmossman · 2022-06-08T23:54:57Z

airbyte-workers/src/main/java/io/airbyte/workers/general/DefaultReplicationWorker.java

+  public DefaultReplicationWorker(final String jobId,
+                                  final int attempt,
+                                  final AirbyteSource source,
+                                  final AirbyteMapper mapper,
+                                  final AirbyteDestination destination,
+                                  final MessageTracker messageTracker,
+                                  final RecordSchemaValidator recordSchemaValidator) {
+    this(jobId, attempt, source, mapper, destination, messageTracker, recordSchemaValidator, null, null);
+  }


This constructor means just means that the non-container-orchestrator deployments will be setting those values to null, right? I think that is okay, since we care more about tracking issues in cloud which only uses container orchestrators. But just wanted to confirm my understanding

lmossman · 2022-06-08T23:57:07Z

airbyte-workers/src/main/java/io/airbyte/workers/general/DefaultReplicationWorker.java

@@ -354,8 +377,18 @@ private static Runnable getReplicationRunnable(final AirbyteSource source,
        }
        LOGGER.info("Total records read: {} ({})", recordsRead, FileUtils.byteCountToDisplaySize(messageTracker.getTotalBytesEmitted()));
        if (!validationErrors.isEmpty()) {
+          DogStatsDMetricSingleton.initialize(MetricEmittingApps.WORKER, new DatadogClientConfiguration(configs));


Instead of passing the entire Configs object into this class just to be used here to create the DatadogClientConfiguration object, I think it would be better to just pass that DatadogClientConfiguration object into this class directly. That way we aren't passing in more than we need.

In fact, I think it would be better if we created a new class to contain all of the logic that you added here. Something like a DatadogSchemaValidationCounter class, with a constructor that takes in the DatadogClientConfiguration object and the sourceDockerImage string (and maybe the constructor can split out the repo and version from the image string into separate class variables), and has a method that called something like track() which contains all of the logic to initialize the DogStatsDMetricSingleton, create the validationErrorMetadata, and call the DogStatsDMetricSingleton.count() method.

Then, I think this class should be wrapped in an Optional<> when passed into this DefaultReplicationWorker class, to make it explicit that it is not always set. It should only be set to something in the ReplicationJobOrchestrator class if the DD_AGENT_HOST and DD_DOGSTATSD_PORT env vars are not empty, so that none of this is attempted for Kube OSS users that have not configured datadog. And the track() method on that class (or whatever you choose to call it) should only be called if the Optional is not empty.

The reason for all of this is that it encapsulates the logic related to datadog tracking into a separate class rather than further complicating the logic in this getReplicationRunnable() method. Also, without an approach like the above I think you would need to add more null checks to the logic you've added here, otherwise I believe this will break when someone hasn't set those DD_* variables, or if someone deploys this on a non-container-orchestrator deployment.

What do you think?

yes that makes sense! when you say the class should be wrapped in Optional<> when it's passed in - is that instead of having multiple constructors?

Yeah I think just having a single constructor would be cleaner

alovew · 2022-06-09T17:58:28Z

@lmossman I updated this (obvs not a priority to look at right now). I made a separate class for datadog metrics, but made it more generic than you were suggesting - I was thinking we might want to track other kinds of metrics in the replication worker and could use that class for other metrics as well. let me know what you think.

benmoriceau · 2022-08-04T22:32:33Z

...r-orchestrator/src/main/java/io/airbyte/container_orchestrator/ContainerOrchestratorApp.java

@@ -55,6 +57,7 @@
 @Slf4j
 public class ContainerOrchestratorApp {

+  private static final Logger LOGGER = LoggerFactory.getLogger(ContainerOrchestratorApp.class);


You don't need that. There is a logger available inthe log variable

benmoriceau · 2022-08-04T22:33:41Z

...r-orchestrator/src/main/java/io/airbyte/container_orchestrator/ContainerOrchestratorApp.java

        System.setProperty(envVar, envMap.get(envVar));
+        final String getEnvResult = System.getenv(envVar);
+        LOGGER.info("getting env " + envVar);
+        LOGGER.info(getEnvResult);


Is it for debug only? This will need to be removed, we may have some credential in the env variables.

benmoriceau · 2022-08-04T22:34:13Z

...orchestrator/src/main/java/io/airbyte/container_orchestrator/ReplicationJobOrchestrator.java


 @Slf4j
 public class ReplicationJobOrchestrator implements JobOrchestrator<StandardSyncInput> {

+  private static final Logger LOGGER = LoggerFactory.getLogger(ReplicationJobOrchestrator.class);


We also have a logger present in the log var

benmoriceau · 2022-08-04T22:40:09Z

airbyte-workers/src/main/java/io/airbyte/workers/process/AsyncOrchestratorPodProcess.java

+    LOGGER.info("metric client in async orchestrator pod process");
+    LOGGER.info(metricClient);
+
+    envVars.add(new EnvVar(EnvConfigs.METRIC_CLIENT, metricClient, null));


Don't you need the Datadog related env vars here?

I thought so but apparently not since this is working without it

'working' as in, the metric client is showing up as a properly initialized datadog metric client

alovew · 2022-08-05T21:08:10Z

Tested this on dev and it's working now, env variables are all correctly being passed to the worker.

jdpgrailsdev

dismissing bc comments were addressed

github-actions bot added area/platform issues related to the platform area/worker Related to worker labels Jun 1, 2022

alovew force-pushed the anne/datadog-tracking-schema-validation branch from 9188dc8 to c6a8f55 Compare June 1, 2022 21:39

alovew temporarily deployed to more-secrets June 1, 2022 21:41 Inactive

alovew temporarily deployed to more-secrets June 2, 2022 17:48 Inactive

alovew force-pushed the anne/datadog-tracking-schema-validation branch from df29a52 to 1e0bd00 Compare June 7, 2022 17:04

alovew temporarily deployed to more-secrets June 7, 2022 17:06 Inactive

alovew temporarily deployed to more-secrets June 7, 2022 21:58 Inactive

alovew temporarily deployed to more-secrets June 8, 2022 16:38 Inactive

alovew requested a review from lmossman June 8, 2022 18:26

lmossman previously requested changes Jun 9, 2022

View reviewed changes

alovew temporarily deployed to more-secrets June 9, 2022 17:52 Inactive

alovew temporarily deployed to more-secrets June 9, 2022 17:59 Inactive

alovew force-pushed the anne/datadog-tracking-schema-validation branch from 79d4fc7 to 1e08c84 Compare June 27, 2022 19:38

alovew temporarily deployed to more-secrets June 27, 2022 19:40 Inactive

alovew temporarily deployed to more-secrets June 27, 2022 21:36 Inactive

alovew force-pushed the anne/datadog-tracking-schema-validation branch from f7a5e50 to b1d70a5 Compare June 27, 2022 22:31

alovew temporarily deployed to more-secrets June 27, 2022 22:33 Inactive

alovew temporarily deployed to more-secrets June 28, 2022 00:15 Inactive

alovew temporarily deployed to more-secrets June 28, 2022 01:11 Inactive

alovew force-pushed the anne/datadog-tracking-schema-validation branch from 51f8fea to bf0a059 Compare June 30, 2022 17:30

alovew temporarily deployed to more-secrets June 30, 2022 17:32 Inactive

alovew force-pushed the anne/datadog-tracking-schema-validation branch from bf0a059 to 164928f Compare July 5, 2022 16:43

alovew temporarily deployed to more-secrets July 5, 2022 16:47 Inactive

alovew force-pushed the anne/datadog-tracking-schema-validation branch from 164928f to 0d9e30b Compare July 21, 2022 20:35

alovew temporarily deployed to more-secrets July 21, 2022 20:37 Inactive

alovew force-pushed the anne/datadog-tracking-schema-validation branch from 0d9e30b to 55b8de7 Compare July 21, 2022 23:18

alovew temporarily deployed to more-secrets July 21, 2022 23:21 Inactive

alovew added 6 commits August 4, 2022 09:13

initialize metric client

90447ac

better logging

0794344

formatting

6774d4d

add metric client env var

fb6a29e

logging

13b7118

update count signature

915fc4a

alovew force-pushed the anne/datadog-tracking-schema-validation branch from 5abb899 to 915fc4a Compare August 4, 2022 16:16

alovew temporarily deployed to more-secrets August 4, 2022 16:19 Inactive

throw error

4543272

alovew temporarily deployed to more-secrets August 4, 2022 16:46 Inactive

alovew temporarily deployed to more-secrets August 4, 2022 17:59 Inactive

benmoriceau previously requested changes Aug 4, 2022

View reviewed changes

add dd host and port

2bb66e1

alovew temporarily deployed to more-secrets August 5, 2022 17:11 Inactive

add publish metrics env var

647949f

alovew temporarily deployed to more-secrets August 5, 2022 18:09 Inactive

alovew added 2 commits August 5, 2022 12:51

remove logging

4206773

update errors

0c4c3fc

alovew temporarily deployed to more-secrets August 5, 2022 19:59 Inactive

alovew requested review from benmoriceau and jdpgrailsdev August 5, 2022 21:00

jdpgrailsdev approved these changes Aug 8, 2022

View reviewed changes

alovew merged commit 12270cc into master Aug 8, 2022

alovew deleted the anne/datadog-tracking-schema-validation branch August 8, 2022 17:15

octavia-squidington-iii mentioned this pull request Aug 10, 2022

Bump Airbyte version from 0.39.42-alpha to 0.40.0-alpha #15493

Merged

erohmensing mentioned this pull request Feb 28, 2023

Tech Spec "Track schema validation errors and create OC issues" #20987

Closed

erohmensing mentioned this pull request Apr 4, 2023

Schema validation error collection refactor #24853

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add datadog tracking to record schema validation errors #13393

Add datadog tracking to record schema validation errors #13393

alovew commented Jun 1, 2022

alovew commented Jun 8, 2022

lmossman left a comment •

edited

Loading

lmossman Jun 8, 2022

alovew Jun 9, 2022

lmossman Jun 8, 2022

lmossman Jun 8, 2022

alovew Jun 9, 2022

lmossman Jun 9, 2022

alovew commented Jun 9, 2022

benmoriceau Aug 4, 2022

benmoriceau Aug 4, 2022

benmoriceau Aug 4, 2022

benmoriceau Aug 4, 2022

alovew Aug 4, 2022

alovew Aug 4, 2022

alovew commented Aug 5, 2022

jdpgrailsdev left a comment

Add datadog tracking to record schema validation errors #13393

Add datadog tracking to record schema validation errors #13393

Conversation

alovew commented Jun 1, 2022

alovew commented Jun 8, 2022

lmossman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alovew commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alovew commented Aug 5, 2022

jdpgrailsdev left a comment

Choose a reason for hiding this comment

lmossman left a comment •

edited

Loading