Add Quartz and Apache Camel preview instrumentations #1899

trask · 2021-10-05T04:29:41Z

Resolves #1897
Resolves #1898

heyams · 2021-10-05T16:10:18Z

...ling/src/main/java/com/microsoft/applicationinsights/agent/internal/init/ConfigOverride.java

@@ -71,11 +71,17 @@ static Config getConfig(Configuration config) {
    if (!config.instrumentation.springScheduling.enabled) {
      properties.put("otel.instrumentation.spring-scheduling.enabled", "false");
    }
+    if (!config.preview.instrumentation.apacheCamel.enabled) {
+      properties.put("otel.instrumentation.apache-camel.enabled", "false");


this is off by default.. should check for enabled and then set it to true?

otel.instrumentation.apache-camel.enabled is true by default

DisabledByDefaultInstrumentation is disabled by default. If it's true, this object doesn't seem right.

I'm not following, can you make a specific suggestion?

heyams · 2021-10-05T16:10:26Z

...ling/src/main/java/com/microsoft/applicationinsights/agent/internal/init/ConfigOverride.java

    if (config.preview.instrumentation.grizzly.enabled) {
      // grizzly instrumentation is off by default
      // TODO (trask) investigate if grizzly instrumentation can be enabled upstream by default now
      properties.put("otel.instrumentation.grizzly.enabled", "true");
    }
+    if (!config.preview.instrumentation.quartz.enabled) {
+      properties.put("otel.instrumentation.quartz.enabled", "false");


heyams · 2021-10-05T16:11:27Z

...ooling/src/main/java/com/microsoft/applicationinsights/agent/internal/statsbeat/Feature.java

@@ -49,7 +49,9 @@
  SPRING_INTEGRATION_DISABLED(19),
  LEGACY_PROPAGATION_DISABLED(20),
  GRIZZLY_DISABLED(21), // preview instrumentation
-  STATSBEAT_DISABLED(22); // disable non-essential statsbeat
+  STATSBEAT_DISABLED(22), // disable non-essential statsbeat
+  QUARTZ_DISABLED(23), // preview instrumentation


QUARTZ_ENABLED?

just following existing convention, do u want to change it for grizzle and spring integration also?

grizzly is disabled by default though. which convention are you referring to?

yup, GRIZZLY_ENABLED makes more sense in this case.

all preview instrumentations are disabled by default

except statsbeat is on by default. check overrideconfig, grizzly check enabled and then set it because its default is false.

Can I suggest that you push commit to this PR to update Statsbeat tracking to the way you would like it to be?

heyams · 2021-10-05T16:11:47Z

...ooling/src/main/java/com/microsoft/applicationinsights/agent/internal/statsbeat/Feature.java

-  STATSBEAT_DISABLED(22); // disable non-essential statsbeat
+  STATSBEAT_DISABLED(22), // disable non-essential statsbeat
+  QUARTZ_DISABLED(23), // preview instrumentation
+  APACHE_CAMEL_DISABLED(24); // preview instrumentation


APACHE_CAMEL_ENABLED?

just following convention, do you want to change it for existing too?

yes, please.

or i can clean up after your pr, just fix yours for now. i think other instrumentations were moved recently from instrumentation to preview.

heyams · 2021-10-05T16:12:18Z

...c/main/java/com/microsoft/applicationinsights/agent/internal/statsbeat/FeatureStatsbeat.java

@@ -129,9 +129,15 @@ void trackConfigurationOptions(Configuration config) {
    }

    // preview instrumentation
+    if (!config.preview.instrumentation.apacheCamel.enabled) {


check enabled not negation?

again, just following convention, let me know if you want to change it for existing too

feature tracking is based on default value.. if it's off by default, we track on; on by default, we track off..
in this case, DisabledByDefaultInstrumentation is off by default.. here only check for false, which seems wrong.

I agree what you suggest would be better, but that's not how current features work, do you want me to change the existing convention?

i think it's because moving config from instrumentation to preview make it confusing.. under instrumentation, it's on by default.. now preview default is off.. once it moves to preview, we change the default value.. that's why feature needs to be updated as well to reflect this change.

@heyams please send a commit to this PR to update statsbeat the way you want it

heyams · 2021-10-05T16:12:26Z

...c/main/java/com/microsoft/applicationinsights/agent/internal/statsbeat/FeatureStatsbeat.java

    if (!config.preview.instrumentation.grizzly.enabled) {
      featureList.add(Feature.GRIZZLY_DISABLED);
    }
+    if (!config.preview.instrumentation.quartz.enabled) {


heyams · 2021-10-05T16:13:16Z

agent/instrumentation/build.gradle.kts

@@ -39,6 +39,7 @@ dependencies {
    exclude("ch.qos.logback", "logback-core")
  }

+  implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-apache-camel-2.20:$otelInstrumentationAlphaVersion")


do we need to add this to the instrumentation list?

isn't that what this does?

INSTRUMENTATION_MAP.put("io.opentelemetry.quartz-2.0", 70); INSTRUMENTATION_MAP.put("io.opentelemetry.apache-camel-2.20", 71);

heyams · 2021-10-05T16:13:26Z

agent/instrumentation/build.gradle.kts

@@ -106,6 +107,7 @@ dependencies {
  implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-opentelemetry-annotations-1.0:$otelInstrumentationAlphaVersion")
  implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-opentelemetry-api-1.0:$otelInstrumentationAlphaVersion")
  implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-opentelemetry-api-metrics-1.0:$otelInstrumentationAlphaVersion")
+  implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-quartz-2.0:$otelInstrumentationAlphaVersion")


instrumentation list?

isn't that what this does?

INSTRUMENTATION_MAP.put("io.opentelemetry.quartz-2.0", 70); INSTRUMENTATION_MAP.put("io.opentelemetry.apache-camel-2.20", 71);

trask · 2021-10-05T20:34:34Z

...ling/src/main/java/com/microsoft/applicationinsights/agent/internal/init/ConfigOverride.java

+    if (config.preview.instrumentation.apacheCamel.enabled) {
+      // apache-camel instrumentation is off by default
+      properties.put("otel.instrumentation.apache-camel.enabled", "true");


this won't work, apache-camel is enabled by default in otel

that's the confusing part.. preview instrumentation is off by default. are you saying that otel will enable it by disregarding the json config setting?

where is the otel code to enable it?
are you saying that, DisabledByDefaultInstrumentation is off by default, but otel turned it on at runtime.

core otel instrumentation is on by default. our configuration layer reverses that for our distro

check out https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/suppressing-instrumentation.md

i only found apache camel and grizzly in that list.. quartz and springIntegration are not mentioned though.

trask · 2021-10-05T20:34:51Z

...ling/src/main/java/com/microsoft/applicationinsights/agent/internal/init/ConfigOverride.java

+    if (config.preview.instrumentation.quartz.enabled) {
+      // quartz instrumentation is off by default
+      properties.put("otel.instrumentation.quartz.enabled", "true");
    }
-    if (!config.preview.instrumentation.springIntegration.enabled) {
-      properties.put("otel.instrumentation.spring-integration.enabled", "false");
+    if (config.preview.instrumentation.springIntegration.enabled) {
+      // springIntegration instrumentation is off by default
+      properties.put("otel.instrumentation.spring-integration.enabled", "true");


same with these

trask · 2021-10-05T21:46:12Z

...ooling/src/main/java/com/microsoft/applicationinsights/agent/internal/statsbeat/Feature.java

-  GRIZZLY_DISABLED(21), // preview instrumentation
+  GRIZZLY_ENABLED(21), // preview instrumentation, grizzly is OFF by default in OTEL


@heyams just a heads up, I'm hoping this changes, and grizzly instrumentation can be on by default in the future, not sure if it changes your thoughts on tracking or not though

that's fine.. when that happens, i can rename to disabled then and update featurestatsbeat accordingly.
we want to track the non-default config settings.

Add Quartz preview instrumentation

c6afa57

trask requested review from heyams, kryalama and xiao-lix as code owners October 5, 2021 04:29

trask added 2 commits October 4, 2021 21:33

And Apache Camel preview instrumentation

fcf5a36

Statsbeat

ca544bc

trask changed the title ~~Add Quartz preview instrumentation~~ Add Quartz and Apache Camel preview instrumentations Oct 5, 2021

heyams reviewed Oct 5, 2021

View reviewed changes

trask and others added 2 commits October 5, 2021 09:41

Update lock files

b224509

Fix feature tracking for preview instrumentations

06f30e8

kryalama approved these changes Oct 5, 2021

View reviewed changes

heyams approved these changes Oct 5, 2021

View reviewed changes

trask commented Oct 5, 2021

View reviewed changes

heyams added 2 commits October 5, 2021 14:36

Fix instrumentations on default by OTEL

a1101f9

Fix spotlessApply

2584919

trask commented Oct 5, 2021

View reviewed changes

heyams merged commit c8f0ba3 into main Oct 5, 2021

trask deleted the add-quartz-preview-instrumentation branch October 18, 2021 00:56

kryalama added this to the 3.2.0 (GA) milestone Oct 21, 2021

		GRIZZLY_DISABLED(21), // preview instrumentation
		GRIZZLY_ENABLED(21), // preview instrumentation, grizzly is OFF by default in OTEL

Add Quartz and Apache Camel preview instrumentations #1899

Add Quartz and Apache Camel preview instrumentations #1899

Conversation

trask commented Oct 5, 2021 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyams Oct 5, 2021 • 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

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

heyams Oct 5, 2021 • 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

heyams Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

trask commented Oct 5, 2021 •

edited

Loading

heyams Oct 5, 2021 •

edited

Loading

heyams Oct 5, 2021 •

edited

Loading

heyams Oct 5, 2021 •

edited

Loading