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 system tests for span links #1811

Merged
merged 8 commits into from Nov 16, 2023
Merged

Add system tests for span links #1811

merged 8 commits into from Nov 16, 2023

Conversation

bwoebi
Copy link
Contributor

@bwoebi bwoebi commented Nov 8, 2023

Description

Add span link tests, covering use cases as outlined in the RFC.

Reviewer checklist

  • Check what scenarios are modified. If needed, add the relevant label (run-parametric-scenario, run-profiling-scenario...). If this PR modifies any system-tests internal, then add the run-all-scenarios label (more info).
  • CI is green
    • If not, failing jobs are not related to this change (and you are 100% sure about this statement)
  • if any of build-some-image label is present
    1. is the image labl have been updated ?
    2. just before merging, locally build and push the image to hub.docker.com
  • if a scenario is added (or removed), add (or remove) it in system-test-dasboard nightly

@bwoebi
Copy link
Contributor Author

bwoebi commented Nov 8, 2023

I think something went wrong with utils/parametric/protos/apm_test_client_pb2.py that there are so many lines removed, but maybe I just don't know the proper incantation to regenerate.

@bwoebi bwoebi force-pushed the bob/span-link-tests branch 2 times, most recently from f7975e9 to 6a0a609 Compare November 8, 2023 17:35
@bwoebi
Copy link
Contributor Author

bwoebi commented Nov 8, 2023

Looks like things are still passing - the only failure is an unrelated nodejs failure.

@bwoebi bwoebi marked this pull request as ready for review November 8, 2023 17:44
@bwoebi bwoebi requested review from a team as code owners November 8, 2023 17:44
tests/parametric/test_span_links.py Outdated Show resolved Hide resolved
@cbeauchesne
Copy link
Collaborator

I think something went wrong with utils/parametric/protos/apm_test_client_pb2.py that there are so many lines removed, but maybe I just don't know the proper incantation to regenerate.

@Kyle-Verhoog, do you have few minutes to help us on that ?

Copy link
Contributor

@PerfectSlayer PerfectSlayer left a comment

Choose a reason for hiding this comment

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

About protocol, do we really want to add span links API to the default API? Can we move them to the OTel API message instead? Because they do are expected to work and are missing and I thought the core/internal API was not ready to use (span kind, etc...).

utils/parametric/protos/apm_test_client.proto Outdated Show resolved Hide resolved
tests/parametric/test_span_links.py Show resolved Hide resolved
tests/parametric/test_span_links.py Outdated Show resolved Hide resolved
tests/parametric/test_span_links.py Show resolved Hide resolved
tests/parametric/test_span_links.py Outdated Show resolved Hide resolved
@Kyle-Verhoog
Copy link
Member

I think something went wrong with utils/parametric/protos/apm_test_client_pb2.py that there are so many lines removed, but maybe I just don't know the proper incantation to regenerate.

@Kyle-Verhoog, do you have few minutes to help us on that ?

https://github.com/DataDog/system-tests/blob/main/utils/parametric/generate_protos.sh should be the script to do it but I think this is broken due to it being moved from the tests/parametric directory.

To regenerate it should be:

python -m grpc_tools.protoc -I. --python_out=. --grpc_python_out=. protos/apm_test_client.proto
sed -i '' -e 's/from protos/from utils.parametric.protos/g' protos/*.py

run from the tests/parametric directory.

@bwoebi
Copy link
Contributor Author

bwoebi commented Nov 13, 2023

@Kyle-Verhoog This doesn't help - it has the same output for me.

@bwoebi bwoebi force-pushed the bob/span-link-tests branch 2 times, most recently from fb54877 to 65bb289 Compare November 13, 2023 19:02
@bwoebi bwoebi requested review from a team as code owners November 13, 2023 19:02
assert span_has_no_parent(span)
assert span["meta"].get(ORIGIN) is None
assert span["meta"].get("_dd.p.dm") == "-4"
assert span["metrics"].get(SAMPLING_PRIORITY_KEY) == 2
Copy link
Contributor

Choose a reason for hiding this comment

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

The last bit of behavior that I think needs to be added to these test cases is the following:

  • Root span ("root") is started with some Datadog information like sampling_priority=2
  • A new span ("processor") is started from distributed tracing headers (e.g. sampling_priority=-1) but we should be adding a span link instead of becoming a child of the distributed trace

Result: ??? Should the sampling_priority and other information be inherited from the root span or from the incoming distributed tracing headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on the integration what makes sense. Either behaviour might be fine - mostly it's going to be inherited from distributed tracing headers, but this is an integration specific choice, and as such not system-test-able.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, makes sense to me!

@PerfectSlayer
Copy link
Contributor

@bwoebi Any thoughts about my comment?

The default API to control the Java tracer in parametric tests is using OpenTracing.
And adding span link support in OpenTracing does not make unanimity.
Can we move over the OTel API instead as do have to support span links for this API?

@bwoebi bwoebi merged commit 1367f9a into main Nov 16, 2023
165 checks passed
@bwoebi bwoebi deleted the bob/span-link-tests branch November 16, 2023 10:30
@bwoebi
Copy link
Contributor Author

bwoebi commented Nov 16, 2023

@PerfectSlayer Missed that comment.

I'm not sure what constraints Java has here. With the PHP implementation of OpenTelemetry and OpenTracing, you could use both APIs together in the same script.

PHP does not plan to add span links to OpenTracing either - the Span Links API the server.php in PR implements is the native API (exposed from C to PHP) - so that we can verify that the exposed primitives work flawlessly.

@PerfectSlayer
Copy link
Contributor

With the PHP implementation of OpenTelemetry and OpenTracing, you could use both APIs together in the same script.

Same in Java. The main set of message is linked to OpenTracing API, while the OTel set of messages is linked to the OTel API.

the Span Links API the server.php in PR implements is the native API (exposed from C to PHP) - so that we can verify that the exposed primitives work flawlessly

The only API (internal or public DD API) of the Java tracer used in the Java parametric client is the flush method.

If you create span using OpenTracing, you will end up with an OpenTracing span (and only the OpenTracing primitives) for which we don't / won't support span links. So I can't retrieve the internal span from the tracer and edit it to add a link to it.

It is similar with OTel, but as OTel span links will be supported, I could then add a span links using the OTel primitives.

So why don't use the OTel set rather than the default (OpenTracing)? Or at least, why not test them both so at least Java can cover one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants