Add Bedrock contract tests. #855
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mxiamxia
force-pushed
the
bedrock_contract_text
branch
from
b124f15 to
aa08642
Compare
mxiamxia
force-pushed
the
bedrock_contract_text
branch
from
aa08642 to
e9d340d
Compare
thpierce
reviewed
Aug 7, 2024
Comment on lines
205
to
206
| void testBedrockGetKnowlesgeBase() { | ||
| doTestBedrockGetKnowlesgeBase(); |
There was a problem hiding this comment.
Not blocking:
Suggested change
| void testBedrockGetKnowlesgeBase() { | |
| doTestBedrockGetKnowlesgeBase(); | |
| void testBedrockGetKnowledgeBase() { | |
| doTestBedrockGetKnowledgeBase(); |
There was a problem hiding this comment.
Not blocking: Missed testBedrockAgentGetKnowlesgeBaseIdFromRequest
...t/java/software/amazon/opentelemetry/appsignals/test/utils/SemanticConventionsConstants.java
Outdated
Show resolved
Hide resolved
...t/java/software/amazon/opentelemetry/appsignals/test/utils/SemanticConventionsConstants.java
Outdated
Show resolved
Hide resolved
.../src/test/java/software/amazon/opentelemetry/appsignals/test/awssdk/base/AwsSdkBaseTest.java
Outdated
Show resolved
Hide resolved
appsignals-tests/images/aws-sdk/aws-sdk-v1/src/main/java/com/amazon/sampleapp/App.java
Outdated
Show resolved
Hide resolved
appsignals-tests/images/aws-sdk/aws-sdk-v2/src/main/java/com/amazon/sampleapp/App.java
Outdated
Show resolved
Hide resolved
thpierce
reviewed
Aug 7, 2024
.../src/test/java/software/amazon/opentelemetry/appsignals/test/awssdk/base/AwsSdkBaseTest.java
Outdated
Show resolved
Hide resolved
|
Is the overview up to date? It mentions changes that I don't see, like |
Seems like this is a rebased PR, and the PR description was not copied correctly. Updated the description from original PR. |
thpierce
approved these changes
Aug 15, 2024
appsignals-tests/images/aws-sdk/aws-sdk-base/src/main/java/com/amazon/sampleapp/Utils.java
Show resolved
Hide resolved
thpierce
approved these changes
Aug 16, 2024
vasireddy99
approved these changes
Aug 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR is a follow up on bedrock service support PR:
#826
We add contract tests for following Bedrock services that covers all resource attributes we newly support:
GetGuardrailGetAgent,GetDataSource,GetKnowledgeBaseInvokeModelInvokeAgentUpgrade
aws-java-sdkV1 and V2 to the latest version so that to support Bedrock services API calls.Upgrade
localstack/localstackimage to the latest version3.5.0so resolve the SQS API call issue usinglocalstack/localstack:2.0.1: localstack/localstack#9610Add
.withEnv("LOCALSTACK_HOST", "127.0.0.1")forLocalStackContainerto workaround the 3.X version limitation: localstack/localstack#9753Contract test limitation:
The contract tests in current repo is using LocalStackContainer to serve AWS SDK service calls. But it doesn't has bedrock related service support (This is the full service list it support.). In this case, no matter which bedrock API we call in contract test, the response will always be 4XX.
As a workaround, we We point all Bedrock related request endpoints to the local app and then specifically handle each request to return the expected response to make sure we receive http response with expected status code and attributes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.