Custom APM Tracing #17947

jdpgrailsdev · 2022-10-13T14:11:15Z

What

Improve runtime performance visibility via APM tracing

How

Add custom APM traces to Temporal workflows in airbyte-workers
Add custom APM traces to Temporal activities in airbyte-workers
Add custom APM traces to scheduled tasks in airbyte-cron

Tests

Unit tests added for ApmTraceUtils.java
Project builds locally without issue
Project runs locally without APM agent present
Deployed to dev environment and configured with APM agent to confirm that traces are generated

N.B. There will be follow-on PR's to enable in cloud and to enable APM tracing in airbyte-container-orchestrator.

gosusnp

The tracing part is fairly manual, I wonder if we should plan for some abstractions to ease it up.
Also, part of it is a bit redundant in a way, I am thinking mostly of having the trace annotations then extra calls from the function to add the tags.

benmoriceau · 2022-10-13T16:53:39Z

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

+   * @param tags A map of tags to be added to the currently active span.
+   * @param tagPrefix The prefix to be added to each custom tag name.
+   */
+  public static void addTagsToTrace(final Span span, final Map<String, Object> tags, final String tagPrefix) {


Why can it be null?

@benmoriceau In the case that the agent is not present, OpenTracing uses a no-op tracer that returns a null span. Therefore, this check is here for the OSS case where no APM agent has been provided.

jdpgrailsdev · 2022-10-13T16:59:40Z

The tracing part is fairly manual, I wonder if we should plan for some abstractions to ease it up. Also, part of it is a bit redundant in a way, I am thinking mostly of having the trace annotations then extra calls from the function to add the tags.

@gosusnp Unfortunately, this is just how custom tracing works. The options are either annotation-based or to manually create a Span via the Tracer if you want a smaller scoped area that is traced than a full method. The tag bit cannot be applied via the annotation, so that has to be done manually as well.

colesnodgrass · 2022-10-13T16:52:48Z

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

+  public static final String TAG_FORMAT = "airbyte.%s.%s";
+
+  /**
+   * Standard prefix for tags added to spans.
+   */
+  public static final String TAG_PREFIX = "metadata";


Do these need to be public?

@colesnodgrass Yes, as it is a class, not an interface. The classes that use this are not in the same package.

colesnodgrass · 2022-10-13T17:14:26Z

airbyte-workers/src/main/java/io/airbyte/workers/temporal/TemporalTraceConstants.java

+/**
+ * Collection of constants for APM tracing of Temporal activities and workflows.
+ */
+public final class TemporalTraceConstants {


These constants seem to fall into two separate categories; operation name and tag key. I'm wondering if we should separate these into two different classes or maybe two different inner classes within this TemporalTraceConstants.

This would have two benefits, the constants are more closely organized within their specific type (tag key vs operation name) and it would help to reduce the verbosity of the current constants while still keeping them within the same file.

TemporalTraceConstants.CONNECTION_ID_TAG_KEY -> TemporalTraceConstants.TagKey.CONNECTION_ID and would be referenced by `TagKey.CONNECTION_ID` where used.

Another thought, we could flip the naming convention here so it becomes TAG_KEY_* instead of *_TAG_KEY to help logically keep all these similar constants grouped together.

@colesnodgrass I don't feel strongly about any of this, so happy to re-organize. I am basically trying to avoid string duplication. That being said, they are all being used for tracing/APM purposes. I would tend to lean towards reversing the name order than having a separate class. Thoughts?

@colesnodgrass I went against my statement above and created a Tags class within the constants class.

* master: (304 commits) Bump helm chart version reference to 0.40.27 (#18152) Bump helm chart version reference to 0.40.26 (#18094) Update deployment.yaml (#18151) Publishes Postgres, MySQL, MSSQL source with changes from #18041 (#18086) Fix minor DBT Cloud Errors. (#18147) Sentry Integration : Stop reporting all non system-error error types. (#18133) Docs: Fix backoff stategy docs (#18143) 🐛 Destination GCS: Fix error logs to log 'Gcs' rather than 'AWS' (#17901) Add openAPI spec for Connector Builder Server (#17535) Alex/mvp UI for dbt cloud integration (#18095) increased timeout for sat tests (#18128) Bmoric/remove dep connector worker (#17977) `recordsRead` should be a long (#18123) doc_update_oath_issue_gsc (#17967) 🎉 Source Zendesk Chat: engagements data fix infinity looping + gradlew format (#18121) 🐛 Source Zendesk Chat: engagements data fix infinity looping (#17745) Custom APM Tracing (#17947) 11679 BigQuery-Denormalized Destination: improve code coverage (#17827) increased timeout for sat tests (#18114) docs: clarify language (#18090) ...

* Add APM tracing to airbyte-workers * Add custom traces to airbyte-cron * Formatting * Refactor tag constants * Handle potential null object * Handle null tag values

jdpgrailsdev requested review from colesnodgrass, gosusnp, benmoriceau and alovew October 13, 2022 14:11

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

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 14:13 Inactive

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 14:32 Inactive

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 14:52 Inactive

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 16:06 Inactive

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 16:09 Inactive

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 16:22 Inactive

gosusnp approved these changes Oct 13, 2022

View reviewed changes

benmoriceau approved these changes Oct 13, 2022

View reviewed changes

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 16:57 Inactive

colesnodgrass reviewed Oct 13, 2022

View reviewed changes

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 18:09 Inactive

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 19:59 Inactive

jdpgrailsdev temporarily deployed to more-secrets October 13, 2022 20:25 Inactive

jdpgrailsdev temporarily deployed to more-secrets October 17, 2022 13:10 Inactive

jdpgrailsdev temporarily deployed to more-secrets October 17, 2022 14:40 Inactive

jdpgrailsdev added 4 commits October 18, 2022 11:54

Add APM tracing to airbyte-workers

711a1b2

Add custom traces to airbyte-cron

427f4cd

Formatting

8aad1f5

Refactor tag constants

3369c49

jdpgrailsdev force-pushed the jonathan/workers-apm-trace branch from 2e85c3e to 3369c49 Compare October 18, 2022 16:00

jdpgrailsdev temporarily deployed to more-secrets October 18, 2022 16:02 Inactive

Handle potential null object

f2c46d4

jdpgrailsdev temporarily deployed to more-secrets October 18, 2022 16:17 Inactive

Handle null tag values

d0a589d

jdpgrailsdev temporarily deployed to more-secrets October 18, 2022 16:23 Inactive

jdpgrailsdev merged commit b4c16bf into master Oct 18, 2022

jdpgrailsdev deleted the jonathan/workers-apm-trace branch October 18, 2022 17:14

octavia-squidington-iii mentioned this pull request Oct 20, 2022

Bump Airbyte version from 0.40.15 to 0.40.16 #18268

Merged

marcosmarxm mentioned this pull request Dec 9, 2022

Airbyte Cron: renable schedule to file cleaner #20317

Merged

37 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom APM Tracing #17947

Custom APM Tracing #17947

jdpgrailsdev commented Oct 13, 2022

gosusnp left a comment

benmoriceau Oct 13, 2022

jdpgrailsdev Oct 13, 2022

jdpgrailsdev commented Oct 13, 2022

colesnodgrass Oct 13, 2022

jdpgrailsdev Oct 13, 2022

colesnodgrass Oct 13, 2022

colesnodgrass Oct 13, 2022

jdpgrailsdev Oct 13, 2022

jdpgrailsdev Oct 13, 2022

Custom APM Tracing #17947

Custom APM Tracing #17947

Conversation

jdpgrailsdev commented Oct 13, 2022

What

How

Recommended reading order

Tests

gosusnp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdpgrailsdev commented Oct 13, 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