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 revision of oteltest #2487

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

pmcollins
Copy link
Member

@pmcollins pmcollins commented May 1, 2024

This is an OpenTelemetry scenario runner for running integration tests and examples. It sets up and runs a scenario defined by each script in a given directory. Script authors define the dependencies, the environment variables, the executable part of the script + any client code, and oteltest sets up the environment, runs the script + client code, and sends the received telemetry back to the script, at which point it can perform assertions.

This was demoed at the Python SIG, and there was an initial PR in the core repo, but it has been recommended that this go into Contrib.

Addresses open-telemetry/opentelemetry-python#3872

@pmcollins pmcollins marked this pull request as ready for review May 1, 2024 16:38
@pmcollins pmcollins requested a review from a team as a code owner May 1, 2024 16:38
@pmcollins
Copy link
Member Author

pmcollins commented May 1, 2024

Also, I published this package to pypi for my own testing purposes: https://pypi.org/project/oteltest/ . Not sure yet what the process is but am happy to transfer its ownership to this SIG.

Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi left a comment

Choose a reason for hiding this comment

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

Thank you @pmcollins !

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

I think that oteltest may use a bit more of unit testing, the Telemetry class methods and some helpers in oteltest.private at least. I'm not asking to add them, just a thought.

@pmcollins
Copy link
Member Author

I think that oteltest may use a bit more of unit testing, the Telemetry class methods and some helpers in oteltest.private at least. I'm not asking to add them, just a thought.

Totally agree. There's some testing that I want to add in later PRs but will add this now as well.

@pmcollins
Copy link
Member Author

Thanks @xrmx -- I've added unit tests and missing telemetry functions where there was a lack of symmetry between the signals.

@pmcollins
Copy link
Member Author

Hi all, any objections to me converting this to draft, opening a PR downstream, and closing this PR once that one has landed? (I'm happy to abort that process at any time to put this functionality here if folks would prefer)

Special thanks to @xrmx and @tammy-baylis-swi for the feedback!

@pmcollins pmcollins marked this pull request as draft May 22, 2024 16:22

for req in oteltest_instance.requirements():
print(f"- Will install requirement: '{req}'")
run_subprocess([pip_path, "install", req])
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you thought on installing all the requirements at once? e.g. requirements = " ".join(oteltest_instance.requirements())

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds like an excellent idea. Will try it.


pip_path = script_venv.path_to_executable("pip")

run_subprocess([pip_path, "install", (wheel_file or "oteltest")])
Copy link
Contributor

@xrmx xrmx May 31, 2024

Choose a reason for hiding this comment

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

oteltest version should probably be pinned to the same version of the one installed outside

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent point. Will fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

This also should be installed after I've installed my requirements since they may have stricter dependencies on opentelemetry packages versions

Copy link
Member Author

@pmcollins pmcollins May 31, 2024

Choose a reason for hiding this comment

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

BTW, there's no reason for us to require that an oteltest script define a class that formally implements the OtelTest base class. Longer term, I intend for the OtelTest base class to be just a convenience so your IDE can fill in the missing methods for you. But if you just have a class containing the name OtelTest, and it doesn't inherit from anything, but it does implement all of the OtelTest methods, I think we could easily consider that a suitable implementation. I'd like to not require that target scripts depend on oteltest at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

(And we could allow methods to be missing as well)

python_script_cmd.insert(0, v.path_to_executable(wrapper_script))

# pylint: disable=R1732
proc = subprocess.Popen(
Copy link
Contributor

@xrmx xrmx Jun 3, 2024

Choose a reason for hiding this comment

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

With oteltest 0.11 I get the following:

- Running python script: test_process_runtime_sent_by_default.py
Traceback (most recent call last):
  File "/home/rm/src/test-otel-python/venvtest/bin/oteltest", line 8, in <module>
    sys.exit(main())
  File "/home/rm/src/test-otel-python/venvtest/lib/python3.10/site-packages/oteltest/main.py", line 42, in main
    run(args.script_dir, args.wheel_file, args.venv_parent_dir)
  File "/home/rm/src/test-otel-python/venvtest/lib/python3.10/site-packages/oteltest/private.py", line 37, in run
    setup_script_environment(temp_dir, script_dir, script, wheel_file)
  File "/home/rm/src/test-otel-python/venvtest/lib/python3.10/site-packages/oteltest/private.py", line 67, in setup_script_environment
    stdout, stderr, returncode = run_python_script(
  File "/home/rm/src/test-otel-python/venvtest/lib/python3.10/site-packages/oteltest/private.py", line 112, in run_python_script
    proc = subprocess.Popen(
  File "/usr/lib/python3.10/subprocess.py", line 971, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.10/subprocess.py", line 1863, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
PermissionError: [Errno 13] Permission denied: '/tmp/tmpcawnrbcp/test_process_runtime_sent_by_default/bin/'

The python_script_cmd is ['/tmp/tmp8frcymoc/test_process_runtime_sent_by_default/bin/', '/tmp/tmp8frcymoc/test_process_runtime_sent_by_default/bin/python', 'tests/integration/test_process_runtime_sent_by_default.py']

On the test class I have

    def wrapper_command(self):
        return "opentelemetry-instrument"

Copy link
Contributor

@xrmx xrmx Jun 3, 2024

Choose a reason for hiding this comment

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

The root cause I guess is load_test_class_for_script loading the imported class instead of the one defined in the test module 😅

Copy link
Contributor

@xrmx xrmx Jun 3, 2024

Choose a reason for hiding this comment

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

Worked around by excluding the name of my class explicitly in is_test_class but that would probably require an argument in oteltest or some magic

Copy link
Member Author

Choose a reason for hiding this comment

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

I was hitting a similar problem (I think) where if the script name (minus ".py") was equal to an available package name, it would fail to load the script. I believe this is fixed now (just pushed here and published to pypi as well).

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, it's a different issue, my problem is with using an intermediate class that satisfies is_test_class checks but it's not a real test case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right. Will see if checking for is not ABC will fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

it will for an ABC class but I don't have an ABC class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, well I just added a check for not isabstract, which I think is good in any case. 😄

Would it be acceptable to mark your intermediate class as ABC? After this change, that should get it to not be recognized as a test class.

Copy link
Contributor

@xrmx xrmx Jun 5, 2024

Choose a reason for hiding this comment

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

Nope, adding an "OtelTest" in value.__name__ clause works for me

)
print_subprocess_result(stdout, stderr, returncode)

filename = get_next_json_file(script_dir, module_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to have an option to avoid saving these files

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Maybe should be off by default and on via a CLI switch. Noted for a later PR.


def num_spans(telemetry) -> int:
out = 0
for req in telemetry.trace_requests:
Copy link
Contributor

@xrmx xrmx Jun 3, 2024

Choose a reason for hiding this comment

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

It may make sense to have a flattened list of span serialized like the ConsoleSpanExporter we have in opentelemetry sdk. That would help a lot with writing asserts on specific attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

All for it. Noted for a later PR.

Catch base exception instead of keyboard interrupt
Explicitly flush print statements
Load script using spec file path to prevent name collisions
Format source files
Bump version to 0.12
Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants