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.

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

Add initial requirements for integration test #630

Closed
wants to merge 11 commits into from

Conversation

reyang
Copy link
Member

@reyang reyang commented Jun 1, 2020

Added initial requirements for integration test.
Removed OpenCensus exporter requirement (I guess it is no longer required?).

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

There needs to be a working prototype of this solution before it is added as a requirement to the spec.

specification/library-guidelines.md Outdated Show resolved Hide resolved

Each language library should use the test harness as part of the CI (continuous integration).

#### Test Harness
Copy link
Member

Choose a reason for hiding this comment

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

In OpenTracing we had a separate test harness framework that was operating purely at the API level. The implementation test had to provide certain introspection capabilities to the framework to validate the behavior (fairly limited since OT was not very prescriptive about the actual behavior). This was especially useful for dynamic languages.

reyang and others added 2 commits June 1, 2020 19:27
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

### Integration Test

The goal of integration test is to make sure:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The goal of integration test is to make sure:
The goal of the integration test is to make sure:

(or plural?)

* The API package and SDK package are designed properly, so another SDK can be used to work with the API package.
* The exporters are functional, and the emitted data meets expectations.
* The context propagation is implemented correctly, so a distributed trace could be built among multiple services.
* Sampling logic is implemented consistently across the services, and across the SDKs.
Copy link
Member

Choose a reason for hiding this comment

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

Across which services?

specification/library-guidelines.md Outdated Show resolved Hide resolved
* The context propagation is implemented correctly, so a distributed trace could be built among multiple services.
* Sampling logic is implemented consistently across the services, and across the SDKs.

A test harness is needed to verify each language library's integration and interoperability with other libraries and backends.
Copy link
Member

Choose a reason for hiding this comment

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

Which other libraries? Do you mean other OpenTelemetry implementations (API + default SDK) in other processes?

specification/library-guidelines.md Outdated Show resolved Hide resolved
specification/library-guidelines.md Outdated Show resolved Hide resolved
#### CI Requirement

Each language library should implement a test service which interacts with the test harness. The test service should include:
* A REST endpoint which is used to receive incoming requests from the harness and other services that are involved during the CI.
Copy link
Member

Choose a reason for hiding this comment

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

Please clarify what you mean with "other services that are involved during the CI". Currently it sounds like this was about CI services like CircleCI or AppVeyor.

specification/library-guidelines.md Outdated Show resolved Hide resolved
specification/library-guidelines.md Outdated Show resolved Hide resolved
specification/library-guidelines.md Outdated Show resolved Hide resolved
@tsloughter
Copy link
Member

I don't understand what is meant by like "using the C++ SDK" with the Python API. This sounds way too specific to Python because I don't see how that is something that would work in most languages -- assuming it works for the Python API?

I was working on an API for a sort of integration tests by having tests run in parallel and compare results. So you'd run the tests against a Python instrumented project and against the C++ project to verify they give the same results. But it is on my other machine, I'll paste in what I was thinking of in a little bit.

@tsloughter
Copy link
Member

Still very rough, but really quick, my idea for integration tests so far looked like:

syntax = "proto3";

import "opentelemetry/proto/common/v1/common.proto";
import "opentelemetry/proto/resource/v1/resource.proto";
import "opentelemetry/proto/trace/v1/trace.proto";

service EndToEndService {
  rpc Export(EndToEndServiceRequest) returns (EndToEndServiceResponse) {}
}

message EndToEndServiceRequest {
  repeated Step steps = 1;
}

message EndToEndServiceResponse {
}

message Step {
  oneof step {
    ForwardTo forward_to = 1;
    SetAttributes set_attributes = 2;
    AddEvents add_events = 3;
    SetStatus set_status = 4;
  }
}

message ForwardTo {
  string host = 1;
  uint32 port = 2;
  repeated Step steps = 3;
}

message SetAttributes {
  repeated opentelemetry.proto.common.v1.AttributeKeyValue attributes = 9;
}

message AddEvents {
  repeated opentelemetry.proto.trace.v1.Span.Event events = 1;
}

message SetStatus {
  opentelemetry.proto.trace.v1.Status status = 1;
}

Export might be a bad name for the method... maybe changing to Forward.

Still a lot to add. Most importantly tracestate and probably just include the existing trace_config proto as part of either the steps or the request.

But I was having a set of workers that are configured to send out the steps to the starting points and a collector that'll just support OTLP and then verify what it receives from each.

Meaning each node would configure an exporter to send to the e2e collector, or have the opentelemetry-collector receive them all and it forward to the e2e collector.

@carlosalberto carlosalberto added the spec:integration-test Related to integration testing label Jun 19, 2020
@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 10, 2020
@reyang reyang self-assigned this Jul 18, 2020
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@carlosalberto carlosalberto requested review from a team August 19, 2020 12:37
carlosalberto and others added 3 commits August 19, 2020 14:37
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
carlosalberto and others added 4 commits August 19, 2020 14:38
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@carlosalberto
Copy link
Contributor

Hey @reyang - went and applied the suggestions that seemed trivial. Do you think you will have time to iterate on the rest of the updates anytime soon?

@github-actions github-actions bot removed the Stale label Aug 20, 2020
@reyang
Copy link
Member Author

reyang commented Aug 26, 2020

Hey @reyang - went and applied the suggestions that seemed trivial. Do you think you will have time to iterate on the rest of the updates anytime soon?

Given there were asks about the proof of concept, I think it makes sense to experiment it in one SDK and come back with some real examples. I will have the integration test in OpenTelemetry .NET and come back to this PR.

@github-actions
Copy link

github-actions bot commented Sep 2, 2020

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions
Copy link

github-actions bot commented Sep 9, 2020

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Sep 9, 2020
@andrewhsu
Copy link
Member

@reyang should this PR be reopened so it can be reviewed and brought in? This is on the list of desired items for otel ga.

@reyang
Copy link
Member Author

reyang commented Oct 23, 2020

@andrewhsu I will reduce the scope and send a smaller PR in order to make progress.

@andrewhsu andrewhsu mentioned this pull request Nov 2, 2020
10 tasks
@reyang reyang deleted the reiley/integration-test branch October 4, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:integration-test Related to integration testing Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants