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

Correlation id utilities (GSI 501) #74

Merged
merged 7 commits into from
Dec 5, 2023

Conversation

TheByronHimes
Copy link
Member

This adds the correlation ID utilities as outlined in the Marabou Stork epic.
This PR only adds the utilities and does not modify the behavior of the Kafka providers (that will come next).
The new stuff:

Functions

  • get_correlation_id()
  • new_correlation_id()
  • set_correlation_id()
  • validate_correlation_id()
  • set_context_var()

Errors

  • CorrelationIdContextError
  • InvalidCorrelationIdError
  • MissingCorrelationIdError
    I think MissingCorrelationIdError could be folded into the InvalidCorrelationIdError as it's only used in set_correlation_id to differentiate between an empty string vs an invalid non-empty string.

Other

  • correlation_id_var: The ContextVar instance that is used to store/access a correlation ID for a context.

mephenor
mephenor previously approved these changes Dec 1, 2023
Copy link
Member

@mephenor mephenor left a comment

Choose a reason for hiding this comment

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

Looks nice. Personally I'd fold the CorrelationIDContext and MissingContextID errors into one, don't see a reason to keep them separate if we don't necessarily need to distinguish between unset and empty ID.

@coveralls
Copy link

coveralls commented Dec 1, 2023

Pull Request Test Coverage Report for Build 7099687386

  • 44 of 45 (97.78%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 90.157%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/hexkit/correlation.py 31 32 96.88%
Totals Coverage Status
Change from base Build 6960935059: 0.3%
Covered Lines: 1374
Relevant Lines: 1524

💛 - Coveralls

@TheByronHimes
Copy link
Member Author

@mephenor I've consolidated the errors then.

mephenor
mephenor previously approved these changes Dec 4, 2023
Copy link
Member

@Cito Cito 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, just some suggestions regarding the testing.

tests/unit/test_correlation.py Show resolved Hide resolved
tests/unit/test_correlation.py Show resolved Hide resolved
tests/unit/test_correlation.py Outdated Show resolved Hide resolved
tests/unit/test_correlation.py Outdated Show resolved Hide resolved
tests/unit/test_correlation.py Outdated Show resolved Hide resolved
tests/unit/test_correlation.py Outdated Show resolved Hide resolved
@TheByronHimes
Copy link
Member Author

@Cito please see the changes in 24dce0f and resolve the conversations if you are satisfied. I'm happy to make further modifications if needed.

@TheByronHimes TheByronHimes requested a review from Cito December 5, 2023 11:27
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

@TheByronHimes TheByronHimes merged commit 5c4f0e6 into main Dec 5, 2023
4 checks passed
@TheByronHimes TheByronHimes deleted the feature/correlation_id_utilities_GSI-501 branch December 5, 2023 13:18
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.

4 participants