Cloud Health Dashboard Step 0: Set up Metrics Registry. #10478

davinchia · 2022-02-19T16:15:59Z

What

Set up a Metrics Registry. The purpose of this registry is to better enforce metrics -> application relationship, metric -> description relationship, provide a central location where folks can understand what metrics OSS AB emits, and enforce some standards.

Past experience has shown me that metrics emission can quickly get out of hand: 1) unclear what is emitted 2) similar metrics emitted in multiple places 3) not clear what metrics corresponds to what application.

This is my attempt to provide a framework for us to operate in.

Let me know if folks think this provides more complexity than is useful.

I've added the KubePodProcess metric in here to demonstrate/test how everything will work in practice.

How

Introduce the AirbyteApplications and the AirbyteMetricsRegistry enums to make it clear what applications are emitting metrics, and what metrics are related to what application.
Revamp the DogStatsDMetricsSingleton to handle publishing when publish is both true and false. Add a test case to confirm this.
Revamp the DogStatsDMetricSingleton to interface with the AirbyteApplication interface and the AirbyteMetricRegistry class to enforce the contracts.
Add the KubePodProcess metric to demonstrate how this is all meant to be used.

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

…cation and metrics, and metrics and description.

…c collection for test.

davinchia · 2022-02-21T11:53:30Z

Confirmed the Kube Pod Process metric is showing up in dev-1

…sh is true or false.

cgardens

Looks good!

I want to call out the env var parsing. It feels like we are going backwards on our initiative to not access env vars from within the codebase. See comment below.

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

cgardens · 2022-02-21T17:09:12Z

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

+ * Enum containing all applications metrics are emitted for. Used to initialize
+ * {@link DogStatsDMetricSingleton#initialize(AirbyteApplication, boolean)}.
+ *
+ * Application Name Conventions:


calling out the convention is great! thank you.

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

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

cgardens · 2022-02-21T17:17:26Z

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

+      throw new RuntimeException("You cannot initialize configuration more than once.");
+    }
+
+    if (!publish) {


it's a little hard for me to understand the different configurations.

line 43: if not publish then create the singleton but it won't publish anywhere

line 49: create singleton specifying dd host and port

line 55: ??? not sure.

it might be useful to have comments either in initialize or javadocs comments for the constructor to understand what each of these configurations mean.

you are right, I've added comments and doc strings. Does this make more sense?

some of this is invalidated by the env var change. please take another look

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

cgardens · 2022-02-21T17:20:44Z

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

+    instancePublish = publish;
+    statsDClient = new NonBlockingStatsDClientBuilder()
+        .prefix(appName)
+        .hostname(System.getenv("DD_AGENT_HOST")) // Matches Airbyte Cloud Datadog env vars.


why are we parsing env variables here? is there a reason we wouldn't pass this through EnvConfigs like we do with other env variables? In october there was a big refactor to make sure we weren't paring env variables anywhere in the code base but centralize it all through EnvConfigs and push it to the top of the application. I want to make sure we don't revert those efforts.

good question. my initial thoughts were to place this in the constructor for convenience and also expose a flexible constructor to allow injecting (keeping our clean up in mind). I was also thinking of 'hiding' the variables for monitoring since they are cloud specific.

on second thought, no reason to keep this away from OSS users. we can put it in the env vars like you pointed out and document this for OSS folks with Datadog. How is that?

made the changes - looks much cleaner now

cgardens · 2022-02-21T17:22:44Z

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

+  }
+
+  /**
+   * Submit a single execution time aggregated locally by the Agent. Use this if approximate stats are


as a developer reading this, it's not clear what we mean for approximate versus precise stats and when i should use which method. can you add more clarity, please?

I've updated this. Is this clearer?

better. i guess i'm still not clear on why i would ever use the non-precise one.

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

…odProcess.java Co-authored-by: Charles <[email protected]>

…ption. Simplify the constructor.

davinchia · 2022-02-22T15:29:26Z

@cgardens I'm going to merge this in first to minimise any git management while I continue working on metrics. Please lmk if you disagree with anything - very happy to make changes.

cgardens · 2022-02-22T17:17:34Z

looks good to me. i had the one open question on when to use the two different time measurements but definitely not blocking

davinchia · 2022-02-22T17:32:21Z

I would use the histogram for the following reasons:

there will only be one agent emitting the type (for accuracy).
control over what kind of percentiles to emit - DD provides set percentiles when the metric is calculated globally.
save cost by saving on number of DD metrics needed.

would it be helpful to add ^ to the docstring?

cgardens · 2022-02-24T01:00:28Z

yes! great. thank you.

Refactor airbyte metric library to better enforce links between appli…

6c8a951

…cation and metrics, and metrics and description.

github-actions bot added area/platform issues related to the platform area/scheduler labels Feb 19, 2022

davinchia changed the title ~~Cloud Health Step 1~~ Cloud Health Dashboard Step 0: Set up Metrics Registry. Feb 21, 2022

Merge remote-tracking branch 'origin' into davinchia/metrics-0

e6ae03a

davinchia temporarily deployed to more-secrets February 21, 2022 07:41 Inactive

Clean up and add docstrings.

cc172b8

github-actions bot added the area/worker Related to worker label Feb 21, 2022

davinchia temporarily deployed to more-secrets February 21, 2022 08:13 Inactive

Refactor and remove AirbyteMetric class. Inject one instance of metri…

e9635ae

…c collection for test.

davinchia temporarily deployed to more-secrets February 21, 2022 08:32 Inactive

davinchia added 2 commits February 21, 2022 17:46

Add metrics conventions.

41d5b17

Add metrics conventions.

5b73334

davinchia temporarily deployed to more-secrets February 21, 2022 09:50 Inactive

Add test for PUBLISH_METRICS env var.

4074d64

davinchia temporarily deployed to more-secrets February 21, 2022 11:27 Inactive

davinchia added 2 commits February 21, 2022 19:36

Checkpoint: Fix comment.

fd46d89

Format.

aae3659

davinchia temporarily deployed to more-secrets February 21, 2022 11:51 Inactive

Add test to make sure Metric Singleton works well regardless if publi…

47499d3

…sh is true or false.

davinchia temporarily deployed to more-secrets February 21, 2022 15:11 Inactive

davinchia temporarily deployed to more-secrets February 21, 2022 15:12 Inactive

davinchia marked this pull request as ready for review February 21, 2022 15:17

davinchia requested a review from pmossman February 21, 2022 15:17

cgardens approved these changes Feb 21, 2022

View reviewed changes

Update airbyte-workers/src/main/java/io/airbyte/workers/process/KubeP…

59e9b26

…odProcess.java Co-authored-by: Charles <[email protected]>

davinchia temporarily deployed to more-secrets February 22, 2022 04:48 Inactive

Respond to PR feedback.

ab62736

davinchia temporarily deployed to more-secrets February 22, 2022 05:29 Inactive

davinchia temporarily deployed to more-secrets February 22, 2022 05:30 Inactive

Respond to PR feedback.

e3c20de

davinchia temporarily deployed to more-secrets February 22, 2022 05:50 Inactive

Add env methods for datadog configuration.

6805ffd

davinchia temporarily deployed to more-secrets February 22, 2022 11:23 Inactive

davinchia temporarily deployed to more-secrets February 22, 2022 11:24 Inactive

Introduce new Datadog configuration object.

f5f59ef

davinchia temporarily deployed to more-secrets February 22, 2022 11:43 Inactive

Switch so singleton does not fail loudly to prevent application disru…

3064a2d

…ption. Simplify the constructor.

davinchia temporarily deployed to more-secrets February 22, 2022 14:37 Inactive

Better docstring.

2f29153

davinchia temporarily deployed to more-secrets February 22, 2022 14:53 Inactive

davinchia temporarily deployed to more-secrets February 22, 2022 14:54 Inactive

davinchia merged commit db4dcdd into master Feb 22, 2022

davinchia deleted the davinchia/metrics-0 branch February 22, 2022 15:29

octavia-squidington-iii mentioned this pull request Feb 22, 2022

Bump Airbyte version from 0.35.32-alpha to 0.35.33-alpha #10546

Merged

Cloud Health Dashboard Step 0: Set up Metrics Registry. #10478

Cloud Health Dashboard Step 0: Set up Metrics Registry. #10478

Uh oh!

Conversation

davinchia commented Feb 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Uh oh!

davinchia commented Feb 21, 2022

Uh oh!

cgardens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davinchia commented Feb 22, 2022

Uh oh!

cgardens commented Feb 22, 2022

Uh oh!

davinchia commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgardens commented Feb 24, 2022

Uh oh!

Uh oh!

davinchia commented Feb 19, 2022 •

edited

Loading

davinchia commented Feb 22, 2022 •

edited

Loading