Skip to content
New issue

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

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

Already on GitHub? # to your account

Set up contract tests #40

Merged
merged 15 commits into from
Feb 9, 2024
Merged

Set up contract tests #40

merged 15 commits into from
Feb 9, 2024

Conversation

thpierce
Copy link
Contributor

@thpierce thpierce commented Feb 6, 2024

In this commit, we are setting up contract tests similar to those in the aws-otel-java-instrumentation repo. Initially, we are just commiting the logic to run the mock collector and a single application that uses the requests library, but the tests are extensible to other frameworks.

There are a number of small differences between these tests and the Java contract tests, most of which are language dependent.

Callouts:

  • I've disabled code style checks on three files: mock_collector_service_pb2_grpc.py, mock_collector_service_pb2.py, andmock_collector_service_pb2.pyi. These are generated GRPC files and no edits to these files are recommended after generating from proto files. I have made no edits to these files after generating with commands described in the mock collector README.
  • requests_server.py is based on the native-http-client/App.java
  • mock_collector_client.py is based on the MockCollectorClient.java and ResourceScopeSignal.kt. Note that a lot of the serialization/deserialization logic is simplified in Python/gRPC.
  • mock-collector classes (e.g. mock_collector_server, mock_collector_metrics_service, etc are based on the mockcollector java classes. Callout that java uses an HTTP server wrapping gRPC servicers, while I'm just using a gRPC server and servicers as it was simpler to do so in Python.
  • app_signals_constants.py note that metric names are all lowercase, which is intentional (java will be fixed in future).
  • contract_test_base.py is based on ContractTestBase.java. One substantial difference is that we are not passing information in about the agent, since both testcontainers and OTEL instrumentation mechanisms are quite different in Java vs Python.
  • requests_test.py is based on BaseHttpClientTest.java and NativeHttpClientTest.java
    • Note that PEER_SERVICE does not appear to be supported by Python
    • Note that OTEL does not instrument basic HTTP Servers, so there are no server spans produced by the application and only client spans are produced requests. This is acceptable from a contract perspective, but does result in AWS_LOCAL_OPERATION being InternalOperation and AWS_SPAN_KIND being LOCAL_ROOT on spans/metrics (in metrics AWS_SPAN_KIND is CLIENT, this is expected)
    • Note that NET_PEER_NAME and NET_PEER_PORT are not populated by requests instrumentation, which appears to be an upstream bug. This results in AWS_REMOTE_SERVICE being UnknownRemoteService
  • pylint: disable summary:
    • invalid-name - only used where I do not have control over the method name (e.g. overrides)
    • broad-exception-caught - Used where we really do want to just catch everything and keep going
    • no-self-use - only used when defining methods that are designed to be overridden by subclasses.
    • no-member - Used for Span.SPAN_KIND_CLIENT - for some reason, pylint cannot detect this constant, likely related to gRPC/proto magic

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@thpierce thpierce requested a review from a team February 6, 2024 01:28
@thpierce thpierce force-pushed the contract-tests branch 3 times, most recently from fc7e93a to 796d034 Compare February 7, 2024 19:38
Copy link
Contributor

@srprash srprash left a comment

Choose a reason for hiding this comment

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

TBH, I did not understand much of the protobuf stuff in the mock collector but I guess if it is working fine then all that must be correct. 😄
Other than that did a sanity check on the code and posted comments/questions wherever I needed clarification. Overall LGTM!

Copy link
Contributor

@srprash srprash left a comment

Choose a reason for hiding this comment

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

LGTM!

In this commit, we are setting up contract tests similar to those in the
aws-otel-java-instrumentation repo. Initially, we are just commiting the
logic to run the mock collector and a single application that uses the
requests library, but the tests are extensible to other frameworks.

There are a number of small differences between these tests and the Java
contract tests, most of which are language dependent.

One callout is that the building and running of these tests is currently
possible, but not rigourously so. In coming commits, we will make github
workflows to do this, and several build-related configurations/etc. are
likely to change.
* Make contract tests runnable from project root
* Use public ecr version of python
* Fix py3.8 issues (dict/list/set not subscriptable, need to use Dict/List/Set)
* Remove `--no-isolation` from set up script (initially added as it solves a local problem with my set up, but causes general problems)
* Remove requests/pyproject.toml
* Improve naming of _WAIT_INTERVAL
* Fix type hint
* Improve comment
When docker build fails, it does so silently. We have to detect the
failure with the bash variable $?, then exit the script. This avoids
silent docker failures.
@XinRanZhAWS
Copy link
Contributor

Not blocking: you mentioned an example application need a pyproject.toml, but your request application don’t contain this. It may be because of this application is too simple, so not blocking anything

@thpierce
Copy link
Contributor Author

thpierce commented Feb 9, 2024

@XinRanZhAWS thanks for the reminder, I had it initially, then removed it because I forgot why I had it in the first place. Good thing I wrote it down, and good thing you reviewed haha. Will add back.

Copy link
Contributor

@XinRanZhAWS XinRanZhAWS left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@thpierce thpierce merged commit 9d6a0a5 into main Feb 9, 2024
@thpierce thpierce deleted the contract-tests branch February 9, 2024 16:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants