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

Logging rework #69

Merged
merged 26 commits into from
Jan 7, 2025
Merged

Conversation

tmcqueen-materials
Copy link
Contributor

The process of debugging OpenMSIStream zombie states, where everything is "running" but message consumption has ceased (probably due to an underlying bug in the rdkafka library, see confluentinc/librdkafka#4916 ) was complicated by the suppression of most log information from the Kafka producers/consumers.

This PR removes the log suppression and propagates log level sets appropriately. Part of doing this is removing a dependency on (undocumented, internal) parts of KafkaCrypto, which made me realize message size accounting associated with heartbeats is incorrect.

Creating as a draft PR to see what the runners think. Requires openmsi/openmsitoolbox#8 .

@davidelbert davidelbert self-assigned this Jan 2, 2025
@davidelbert
Copy link
Contributor

I merged the openmsitoolbox PR so the OpenMSILogger class is now updated. Would you please run the run-tests-no-kafka test again to clear the checks before I merge? (I hope that is all it was.). Thanks.

@tmcqueen-materials
Copy link
Contributor Author

@davidelbert : thanks. That did not fix it because although a new v1.2.4 was tagged for openmsitoolbox, it was not pushed to PyPI (and the code version string was not updated beyond v1.2.3). I've submitted a second PR over there for that: openmsi/openmsitoolbox#10 , but not sure the process to issue a new PyPI release.

@davidelbert
Copy link
Contributor

@tmcqueen-materials, openmsitoolbox has new version on PyPi now. It's tagged "openmsitoolbox 1.2.4.post1" as you suggested. Are you now able to rerun the CircleCI pipeline? Thanks.

@Xarthisius
Copy link
Contributor

@tmcqueen-materials you'll also have to bump version of openmsistream itself to make tests pass, e.g.:

diff --git a/openmsistream/version.py b/openmsistream/version.py
index 320141d..ef4e23e 100644
--- a/openmsistream/version.py
+++ b/openmsistream/version.py
@@ -1 +1 @@
-__version__ = "1.8.2"
+__version__ = "1.8.3.dev1"

@tmcqueen-materials
Copy link
Contributor Author

@Xarthisius : Thanks. It looks like this has surfaced a different long-standing nit in the ssl ca location handling code that I'm going to go fix now...

@tmcqueen-materials tmcqueen-materials marked this pull request as ready for review January 6, 2025 23:02
@tmcqueen-materials
Copy link
Contributor Author

@Xarthisius , @davidelbert : At this point, this should be ready for review. The failing tests are associated with what looks like a CI issue and trying to test S3 bucket functionality. Let me know of any changes that are needed.

Copy link
Contributor

@Xarthisius Xarthisius left a comment

Choose a reason for hiding this comment

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

LGTM! I would only add explicit version dependency to:

https://github.com/openmsi/openmsistream/blob/9df39e5cf5ee798e026edf4f3330495c1c824b66/setup.py#L62C9-L62C33

so that it picks up openmsitoolbox with proper changes.

@tmcqueen-materials
Copy link
Contributor Author

@Xarthisius : great idea. Just pushed that change.

@davidelbert
Copy link
Contributor

Merging without the python matrix CircleCI tests passing. They are failing because of lack of S3 bucket writing test endpoint that I can fix later once JetStream2 is back from service or the local allocations are up.

@davidelbert davidelbert merged commit 8312629 into openmsi:main Jan 7, 2025
3 of 6 checks passed
@tmcqueen-materials tmcqueen-materials deleted the logging-rework branch January 7, 2025 14:44
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.

3 participants