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

RUST-2030 Add more event fields: lsid, txnNumber and disambiguatedPaths #1197

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

arthurprs
Copy link
Contributor

Add more event fields: lsid, txnNumber and disambiguatedPaths

lsid: Some(lsid),
txn_number: Some(1),
..
}) if key == doc! { "_id": 1 } && lsid.get("id") == session_id.as_ref()
Copy link
Contributor Author

@arthurprs arthurprs Sep 5, 2024

Choose a reason for hiding this comment

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

The lsid contains a second field "uid", which I'm not familiar with. But the test should still be valid.

@isabelatkinson
Copy link
Contributor

Hey @arthurprs, thanks for opening this PR! We have an open ticket for adding the disambiguated_paths field (RUST-1423) which also includes an option and some additional testing, so we can certainly address that.

What is your use case for adding the txn_number and lsid fields? These are not part of the specification for the ChangeStreamEvent type, so I'm hesitant to make this addition. Would it be possible for you to retrieve these values from CommandStartedEvents emitted via command monitoring/logging instead? They would both be present as top-level fields in the document field for events emitted during a transaction.

@arthurprs
Copy link
Contributor Author

arthurprs commented Sep 6, 2024

Maybe I'm misinterpreting the manual page but lsid and txn_number seemed documented and even referred to by clusterTime description.

Our use cases includes detecting if a certain actor was the source of the write by its session ID (or previous ones).

@abr-egn
Copy link
Contributor

abr-egn commented Sep 10, 2024

You're quite right, those fields are in the manual! The confusing part here on our end is that they're not part of the specification, so I'm working to track down that discrepancy. My concern is that I don't want to add fields to the public API if they're not actually intended to be part of the stable interface 🙂

While we're tracking this down, you should be able to unblock anything that needs those fields by defining your own event type and using it with ChangeStream::with_type.

@abr-egn abr-egn changed the title Add more event fields: lsid, txnNumber and disambiguatedPaths RUST-2030 Add more event fields: lsid, txnNumber and disambiguatedPaths Sep 10, 2024
@abr-egn abr-egn self-requested a review September 11, 2024 16:33
Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

LGTM!

@abr-egn
Copy link
Contributor

abr-egn commented Sep 11, 2024

Sorry for the delay here, it looks like it was just a simple oversight that those fields weren't included in the spec. I've started the test run and assuming there are no unexpected failures I'll merge this in. Thanks again for your patience!

@abr-egn
Copy link
Contributor

abr-egn commented Sep 11, 2024

It looks like the test is using some logic that's not supported in server version 4.x, can you add a check to restrict it to >=5.0?

@arthurprs
Copy link
Contributor Author

arthurprs commented Sep 11, 2024

Done in bba9d00

@abr-egn abr-egn merged commit 13e0635 into mongodb:main Sep 12, 2024
12 of 15 checks passed
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