Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Quartz and Apache Camel preview instrumentations #1899

Merged
merged 7 commits into from
Oct 5, 2021
Merged

Add Quartz and Apache Camel preview instrumentations #1899

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c6afa57
Add Quartz preview instrumentation
trask Oct 5, 2021
fcf5a36
And Apache Camel preview instrumentation
trask Oct 5, 2021
ca544bc
Statsbeat
trask Oct 5, 2021
b224509
Update lock files
trask Oct 5, 2021
06f30e8
Fix feature tracking for preview instrumentations
heyams Oct 5, 2021
a1101f9
Fix instrumentations on default by OTEL
heyams Oct 5, 2021
2584919
Fix spotlessApply
heyams Oct 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...in/java/com/microsoft/applicationinsights/agent/internal/configuration/Configuration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,9 @@ public static class InternalConfiguration {
}

public static class PreviewInstrumentation {

public DisabledByDefaultInstrumentation apacheCamel = new DisabledByDefaultInstrumentation();

// this is just here to detect if using this old setting in order to give a helpful message
@Deprecated
public DisabledByDefaultInstrumentation azureSdk = new DisabledByDefaultInstrumentation();
Expand All @@ -280,6 +283,8 @@ public static class PreviewInstrumentation {
@Deprecated
public DisabledByDefaultInstrumentation jaxws = new DisabledByDefaultInstrumentation();

public DisabledByDefaultInstrumentation quartz = new DisabledByDefaultInstrumentation();

// this is just here to detect if using this old setting in order to give a helpful message
@Deprecated
public DisabledByDefaultInstrumentation rabbitmq = new DisabledByDefaultInstrumentation();
Expand Down
6 changes: 6 additions & 0 deletions ...g/src/main/java/com/microsoft/applicationinsights/agent/internal/init/ConfigOverride.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

}
if (!config.preview.instrumentation.springIntegration.enabled) {
properties.put("otel.instrumentation.spring-integration.enabled", "false");
}
Expand Down
4 changes: 3 additions & 1 deletion ...ing/src/main/java/com/microsoft/applicationinsights/agent/internal/statsbeat/Feature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ enum Feature {
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QUARTZ_ENABLED?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, GRIZZLY_ENABLED makes more sense in this case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all preview instrumentations are disabled by default

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

APACHE_CAMEL_DISABLED(24); // preview instrumentation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APACHE_CAMEL_ENABLED?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, please.

Copy link
Contributor

@heyams heyams Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


private static final Map<String, Feature> javaVendorFeatureMap;

Expand Down
6 changes: 6 additions & 0 deletions ...ain/java/com/microsoft/applicationinsights/agent/internal/statsbeat/FeatureStatsbeat.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,15 @@ void trackConfigurationOptions(Configuration config) {
}

// preview instrumentation
if (!config.preview.instrumentation.apacheCamel.enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check enabled not negation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

featureList.add(Feature.APACHE_CAMEL_DISABLED);
}
if (!config.preview.instrumentation.grizzly.enabled) {
featureList.add(Feature.GRIZZLY_DISABLED);
}
if (!config.preview.instrumentation.quartz.enabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

featureList.add(Feature.QUARTZ_DISABLED);
}
if (!config.preview.instrumentation.springIntegration.enabled) {
featureList.add(Feature.SPRING_INTEGRATION_DISABLED);
}
Expand Down
2 changes: 2 additions & 0 deletions ...ain/java/com/microsoft/applicationinsights/agent/internal/statsbeat/Instrumentations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ class Instrumentations {
INSTRUMENTATION_MAP.put("io.opentelemetry.jaxrs-client-2.0", 67);
INSTRUMENTATION_MAP.put("io.opentelemetry.jaxrs-client-2.0-resteasy-3.0", 68);
INSTRUMENTATION_MAP.put("io.opentelemetry.grizzly-2.0", 69);
INSTRUMENTATION_MAP.put("io.opentelemetry.quartz-2.0", 70);
INSTRUMENTATION_MAP.put("io.opentelemetry.apache-camel-2.20", 71);
}

// encode BitSet to a long
Expand Down
2 changes: 2 additions & 0 deletions agent/instrumentation/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ dependencies {
exclude("ch.qos.logback", "logback-core")
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to add this to the instrumentation list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-apache-httpasyncclient-4.1:$otelInstrumentationAlphaVersion")
implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-apache-httpclient-2.0:$otelInstrumentationAlphaVersion")
implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-apache-httpclient-4.0:$otelInstrumentationAlphaVersion")
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instrumentation list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-rabbitmq-2.7:$otelInstrumentationAlphaVersion")
implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-reactor-3.1:$otelInstrumentationAlphaVersion")
implementation("io.opentelemetry.javaagent.instrumentation:opentelemetry-javaagent-reactor-netty-0.9:$otelInstrumentationAlphaVersion")
Expand Down