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.

Sign up for GitHub

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

Jump to bottom

Delete duplicated OTLP Exporter tests, move them to the mixin unit test. Fix broken shutdown unit test. #4504

Merged
merged 16 commits into from
Apr 4, 2025
Merged

Delete duplicated OTLP Exporter tests, move them to the mixin unit test. Fix broken shutdown unit test. #4504

merged 16 commits into from
Apr 4, 2025
+362 −927

Conversation

DylanRussell
Copy link
Contributor

@DylanRussell DylanRussell commented Mar 24, 2025 •
edited
Loading

Description

Move all OTLP exporter tests that are testing the underlying behavior in the mixin to the mixin unit tests,
instead of having them specified multiple times in the Metric/Log/Trace exporters.

Fix the shutdown tests which were flaky, so that they just test whether a pending export call completes or not.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

This change is just a refactor of the unit tests, to delete duplicate tests and fix a broken test.. No functionality is changing.

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • I don't think a changelog update is necessary for this, but I can add one if needed.
  • Unit tests have been added
  • Documentation has been updated

@DylanRussell
Move all OTLP exporter tests that are testing the
f940175 
underlying behavior in the mixin to the mixin unit
tests, instead of having them specified multiple
times in the Metric/Log/Trace exporters.

Fix the shutdown tests which were flaky, so that
they just test whether a pending export call completes
or not.  Update shutdown so it doesn't release the
lock -- in cases where an export call is pending,
export then also releases the lock causing a Runtime
Error: https://docs.python.org/3/library/threading.html#threading.Lock.release.
@DylanRussell DylanRussell requested a review from a team as a code owner March 24, 2025 20:40
@xrmx
Copy link
Contributor

xrmx commented Mar 26, 2025

@DylanRussell have you seen #4490?

@DylanRussell
Use threading.Event() to communicate when an
fe498ed 
export RPC is occuring, so that shutdown waits for
export RPC to finish.

Use threading.Event() to communicate when shutdown
is occuring, so that sleep is interrupted if a
shutdown is occuring.
@DylanRussell
Copy link
Contributor Author

Ah I had not seen that ! Seems like a couple of us noticed the same thing.

It sounds like the behavior that what we want is for Shutdown() to not interrupt any in progress export RPC, but also for it to prevent any additional retries and also we would like Shutdown() to interrupt the sleep call in export if possible.

I'm thinking we can use threading events (https://docs.python.org/3/library/threading.html#event-objects) to accomplish all these things. We use one event for export to communicate to shutdown that an RPC is in progress, and we use another event for shutdown to communicate to export that shutdown is happening.

Check out my latest commit and let me know what you think !

@DylanRussell
Copy link
Contributor Author

I reverted all changes to the exporter shutdown logic from this PR. So this PR only touches tests now, and is just about deleting duplicate tests and fixing the existing broken shutdown test.

I moved my proposed shutdown changes to a new pr: #4511

aabmass
aabmass reviewed Mar 28, 2025
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Did you mostly rewrite the tests in exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_exporter_mixin or does it copy from the old tests that were deleted here?

@DylanRussell
Copy link
Contributor Author

I copied the existing tests. The only tests I changed were the shutdown tests (test_shutdown.*) because they were flaky / broken

aabmass
aabmass approved these changes Apr 2, 2025
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

If you can make the tests work without sleeping, it would be nice. If not, can you try to shorten the sleep durations a bit?

@DylanRussell DylanRussell mentioned this pull request Apr 3, 2025
Open
xrmx
xrmx approved these changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@xrmx xrmx left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like the reduction of work done, hopefully we are still confident that the exporters behave like the mixin 😅 A couple of nits but LGTM.

@xrmx
Copy link
Contributor

xrmx commented Apr 3, 2025

@jomcgi care to take a look at this too please?

jomcgi
jomcgi approved these changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@jomcgi jomcgi left a comment

Choose a reason for hiding this comment

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

Looks good to me! left some small nits.

@DylanRussell
Copy link
Contributor Author

I think I need someone to add the Skip Changelog label

@aabmass aabmass added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Apr 4, 2025
@aabmass aabmass merged commit 7090f38 into open-telemetry:main Apr 4, 2025
386 of 387 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emdneto emdneto emdneto left review comments

@xrmx xrmx xrmx approved these changes

@aabmass aabmass aabmass approved these changes

@jomcgi jomcgi jomcgi approved these changes

Assignees
No one assigned
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DylanRussell @xrmx @aabmass @emdneto @jomcgi