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

Preventing trace breaking in Otel java 2.0 upgrade #728

Closed
wants to merge 2 commits into from
Closed

Preventing trace breaking in Otel java 2.0 upgrade #728

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
946b244
make grpc protocal to be default
wangzlei Jan 24, 2024
1c1243f
change trace exporter trasnport protocol from grpc to http/protobuf
wangzlei Jan 30, 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
23 changes: 23 additions & 0 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 @@ -34,6 +34,8 @@
import io.opentelemetry.sdk.trace.export.SpanExporter;
import io.opentelemetry.sdk.trace.samplers.Sampler;
import java.time.Duration;
import java.util.HashMap;
import java.util.Map;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand All @@ -57,11 +59,32 @@ public class AwsAppSignalsCustomizerProvider implements AutoConfigurationCustomi
Logger.getLogger(AwsAppSignalsCustomizerProvider.class.getName());

public void customize(AutoConfigurationCustomizer autoConfiguration) {
autoConfiguration.addPropertiesCustomizer(this::customizeProperties);
autoConfiguration.addSamplerCustomizer(this::customizeSampler);
autoConfiguration.addTracerProviderCustomizer(this::customizeTracerProviderBuilder);
autoConfiguration.addSpanExporterCustomizer(this::customizeSpanExporter);
}

private Map<String, String> customizeProperties(ConfigProperties configProps) {
Map<String, String> overrides = new HashMap<>();
if (isSmpEnabled(configProps)) {
// disable logs exporter by default
if (System.getProperty("otel.logs.exporter", System.getenv("OTEL_LOGS_EXPORTER")) == null) {
overrides.put("otel.logs.exporter", "none");
}
Comment on lines +71 to +74
Copy link
Contributor

@thpierce thpierce Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please add details about why to the overview. In general, the overview/commit message is out of date with the new approach. Lets bring it up to date to clarify what we are doing and (more importantly) why.


String traceEndpoint =
System.getProperty(
"otel.exporter.otlp.traces.endpoint",
System.getenv("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"));
// using grpc if trace endpoint is :4315
if (traceEndpoint != null && traceEndpoint.contains(":4315")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add a logline similar to how we callout changes to exportInterval.

overrides.put("otel.exporter.otlp.traces.protocol", "grpc");
Comment on lines +81 to +82
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this assumption is appropriate. The specification provides a mechanism for specifying the protocol to use independent of the endpoint. A customer may have reason to utilize HTTP/JSON over port 4315 and this weird opaque rejection of their intent is not good.

Copy link
Contributor Author

@wangzlei wangzlei Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If SMP flag is enabled, it means customer uses CWAgent for receiving AppSignals metrics. The AppSignals and CWAgent convention is using 4315 for grpc, 4316 for http. These 2 ports are already used and hardcoded by CWAgent.
So, if both SMP is enabled and SDK uses endpoint 4315, it must be grpc protocol.

If CWAgent breaks the contract, does not use 4315 grpc 4316 http in the future, it is a breaking change for all of exisitng AppSignals SDKs running on EKS/ECS/EC2. If it happens, the only way is bump both SDKs and CWAgent major version and call out in user instruction.

}
}
return overrides;
}

private boolean isSmpEnabled(ConfigProperties configProps) {
return configProps.getBoolean("otel.smp.enabled", false);
}
Expand Down
Loading