Remove AWS SDK v1 Dependency from X-Ray SDK Core #428
Conversation
| snapshot = SamplingStatisticsDocument.newBuilder() | ||
| .setClientId(CentralizedSamplingStrategy.getClientID()) | ||
| .setBorrowCount(snapshot.getBorrowCount()) | ||
| .setRequestCount(snapshot.getRequestCount()) | ||
| .setRuleName(snapshot.getRuleName()) | ||
| .setSampledCount(snapshot.getSampledCount()) | ||
| .setTimestamp(snapshot.getTimestamp()) | ||
| .build(); |
There was a problem hiding this comment.
Confused as to why the properties being read and set from the same snapshot instance?
There was a problem hiding this comment.
What's the motivation for this change?
There was a problem hiding this comment.
In AWS SDK V2, a key change is immutability. The one returned from rule.snapshot(date) is read-only. We need create a copy of that, and override snapshot. Let me rename it.
| @Deprecated | ||
| public RulePoller(CentralizedManifest manifest, AWSXRay unused, Clock clock) { | ||
| public RulePoller(CentralizedManifest manifest, Object unused, Clock clock) { | ||
| this(new UnsignedXrayClient(), manifest, clock); | ||
| } |
There was a problem hiding this comment.
non-blocking: Can this constructor be removed entirely since it is deprecated?
| logger.info("Polling sampling rules."); | ||
| GetSamplingRulesRequest req = new GetSamplingRulesRequest(); | ||
| GetSamplingRulesResult records = client.getSamplingRules(req); | ||
| GetSamplingRulesRequest req = GetSamplingRulesRequest.create(null); |
There was a problem hiding this comment.
Passing in a null is generally not a good practice. What does the GetSamplingRulesRequest.create method expects and why are we unable to provide that?
There was a problem hiding this comment.
Here is the definition of create() method: the parameter is @nullable
public static GetSamplingRulesRequest create(@Nullable String nextToken) {
return new AutoValue_GetSamplingRulesRequest(nextToken);
}
| @Deprecated | ||
| public TargetPoller(CentralizedManifest manifest, AWSXRay unused, Clock clock) { | ||
| public TargetPoller(CentralizedManifest manifest, Object unused, Clock clock) { | ||
| this(new UnsignedXrayClient(), manifest, clock); | ||
| } |
There was a problem hiding this comment.
same comment as above. See if this can be removed.
There was a problem hiding this comment.
Method deleted.
There was a problem hiding this comment.
Just curious: Is this and the other new data model classes based off of existing classes in AWS SDK or somewhere else?
There was a problem hiding this comment.
We have similar models in otel-contrib.
| .withCredentials(ANONYMOUS_CREDENTIALS) // This will entirely skip signing too | ||
| .build(); | ||
|
|
||
| public static Object newClient() { |
There was a problem hiding this comment.
Can even be fully removed?
build.gradle.kts
Outdated
| // 1. Google AutoValue does not generate @Nullable or @NonNull annotations on methods like equals() and hashCode(). | ||
| // 2. The existing codebase does not consistently perform null checks, in line with the behavior of the X-Ray SDK: | ||
| // https://docs.aws.amazon.com/xray/latest/api/API_SamplingTargetDocument.html | ||
| skipCheckerFramework = skipCheckerFramework || project.name == "aws-xray-recorder-sdk-core" |
There was a problem hiding this comment.
This conflicts with the condition above where we skip null checker on all projects except the aws-xray-recorder-sdk-core. And now we are disabling on this project itself. What's even the point of having this checker now?
There was a problem hiding this comment.
Good point. I will remove it.
The X-Ray SDK Core previously relied on AWS SDK v1 solely to support AWS Remote Sampling. However, the SDK does not use the SDK’s client to call the sampling service directly. Instead, it composes REST requests manually and sends them through an AWS proxy. The only part of AWS SDK v1 being used was the data model types for serialization into JSON. This change removes the AWS SDK v1 dependency by reimplementing the necessary data types internally, eliminating the need to rely on the external SDK. Test Plan: Unit tests: Pass E2E tests with X-Ray Daemon: Pass E2E tests with CloudWatch Agent: Pass
2 participants
The X-Ray SDK Core previously relied on AWS SDK v1 solely to support AWS Remote Sampling. However, the SDK does not use the SDK’s client to call the sampling service directly. Instead, it composes REST requests manually and sends them through an AWS proxy. The only part of AWS SDK v1 being used was the data model types for serialization into JSON.
This change removes the AWS SDK v1 dependency by reimplementing the necessary data types internally, eliminating the need to rely on the external SDK.
Test Plan:
Unit tests: Pass
E2E tests with X-Ray Daemon: Pass
E2E tests with CloudWatch Agent: Pass
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.