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

Rename AppSignals configs with backward compatibility #744

Merged
merged 3 commits into from
Feb 13, 2024
Merged

Rename AppSignals configs with backward compatibility #744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
74ecaf9
Rename AppSignals configs with backwards compatibility
srprash Feb 8, 2024
dc8ac3e
update config name in contract tests
srprash Feb 8, 2024
254f823
fixing related javadoc
srprash Feb 9, 2024
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
4 changes: 2 additions & 2 deletions ...ts/src/test/java/software/amazon/opentelemetry/appsignals/test/base/ContractTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,11 @@ public abstract class ContractTestBase {
.waitingFor(getApplicationWaitCondition())
.withEnv("JAVA_TOOL_OPTIONS", "-javaagent:/opentelemetry-javaagent-all.jar")
.withEnv("OTEL_METRIC_EXPORT_INTERVAL", "100") // 100 ms
.withEnv("OTEL_SMP_ENABLED", "true")
.withEnv("OTEL_AWS_APP_SIGNALS_ENABLED", "true")
.withEnv("OTEL_METRICS_EXPORTER", "none")
.withEnv("OTEL_BSP_SCHEDULE_DELAY", "0") // Don't wait to export spans to the collector
.withEnv(
"OTEL_AWS_SMP_EXPORTER_ENDPOINT",
"OTEL_AWS_APP_SIGNALS_EXPORTER_ENDPOINT",
"http://" + COLLECTOR_HOSTNAME + ":" + COLLECTOR_PORT)
.withEnv(
"OTEL_EXPORTER_OTLP_TRACES_ENDPOINT",
Expand Down
11 changes: 7 additions & 4 deletions ...va/software/amazon/opentelemetry/javaagent/providers/AwsAppSignalsCustomizerProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,9 @@
* <li>Add AwsMetricAttributesSpanExporter to add more attributes to all spans.
* </ul>
*
* <p>You can control when these customizations are applied using the property otel.smp.enabled or
* the environment variable OTEL_SMP_ENABLED. This flag is enabled by default.
* <p>You can control when these customizations are applied using the property
* otel.aws.app.signals.enabled or the environment variable OTEL_AWS_APP_SIGNALS_ENABLED. This flag
* is disabled by default.
*/
public class AwsAppSignalsCustomizerProvider implements AutoConfigurationCustomizerProvider {
private static final Duration DEFAULT_METRIC_EXPORT_INTERVAL = Duration.ofMinutes(1);
Expand All @@ -63,7 +64,8 @@ public void customize(AutoConfigurationCustomizer autoConfiguration) {
}

private boolean isSmpEnabled(ConfigProperties configProps) {
return configProps.getBoolean("otel.smp.enabled", false);
return configProps.getBoolean(
"otel.aws.app.signals.enabled", configProps.getBoolean("otel.smp.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.

Lets update the comment above AwsAppSignalsCustomizerProvider, too:

 * <p>You can control when these customizations are applied using the property otel.smp.enabled or
* the environment variable OTEL_SMP_ENABLED. This flag is enabled by default.

to

 * <p>You can control when these customizations are applied using the property aws.app.signals,enabled or
* the environment variable OTEL_AWS_APP_SIGNALS_ENABLED. This flag is enabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. done

}

private Sampler customizeSampler(Sampler sampler, ConfigProperties configProps) {
Expand All @@ -79,7 +81,8 @@ private SdkTracerProviderBuilder customizeTracerProviderBuilder(
logger.info("Span Metrics Processor enabled");
String smpEndpoint =
configProps.getString(
"otel.aws.smp.exporter.endpoint", "http://cloudwatch-agent.amazon-cloudwatch:4317");
"otel.aws.app.signals.exporter.endpoint",
configProps.getString("otel.aws.smp.exporter.endpoint", "http://localhost:4315"));
Comment on lines +84 to +85
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, http://cloudwatch-agent.amazon-cloudwatch:4317 as default exporter endpoint doesn't really work since it points to CW pod in EKS environment.

I recall "cloudwatch-agent.amazon-cloudwatch" being an intentional decision. Can you elaborate more on why we want to switch to localhost? You say it doesn't work because "it points to CW pod in EKS environment." I'm not following the argument.

Plus 4317 is also wrong as we have switched to 4315 for AppSignals.

Can you provide a reference for this claim? Public docs/etc? Also this seems to be somewhat tied to HTTP/gRPC changes discussed in #728

IMO, it would be better to untangle these somewhat controversial changes from the (hopefully) not controversial change to property names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try to explain this in 2 parts:

  1. The cloudwatch-agent.amazon-cloudwatch resolves to the CWAgent's K8s pod which is created by the CWOperator, which will also set the "OTEL_AWS_SMP_EXPORTER_ENDPOINT" environment variable. In this case, the default value will never be utilized. So, IMO the default value should be set as per the environments where the above environment needs to be set by users such as EC2, therefore using localhost makes more sense to me.
  2. Since ports 4317 and 4318 are used by OTel, for AppSignals we use 4315 (grpc) and 4316 (http).

So the default address http://localhost:4315 is for users running CWAgent for AppSignals but maybe forgot to manually set the OTEL_AWS_SMP_EXPORTER_ENDPOINT config on their application.

Copy link
Contributor

@thpierce thpierce Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hm I see the argument. This aligns with public documentation which tells customers to manually set OTEL_AWS_SMP_EXPORTER_ENDPOINT to http://localhost:4315 (ECS instructions set to 127.0.0.1, which should resolve to the same thing?) while EKS instructions [1][2] does not reference this variable. Seems reasonable to me.

Duration exportInterval =
configProps.getDuration("otel.metric.export.interval", DEFAULT_METRIC_EXPORT_INTERVAL);
logger.log(Level.FINE, String.format("Span Metrics endpoint: %s", smpEndpoint));
Expand Down
Loading