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

[ADDED] Import/Export.AllowTrace and Account.TraceDest #216

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

kozlovic
Copy link
Member

This is needed for the NATS Server distributed message tracing feature.

This PR updates validation of a Subject to check for . at the first or last position and consecutive ones.

The validation of the Account.TraceDest also checks that the destination does not have wildcards since this is supposed to be a publish subject.

The Import/Export validation ensures that AllowTrace is used by the correct type (service or stream)

Signed-off-by: Ivan Kozlovic [email protected]

This is needed for the NATS Server distributed message tracing
feature.

This PR updates validation of a Subject to check for `.` at the
first or last position and consecutive ones.

The validation of the Account.TraceDest also checks that the
destination does not have wildcards since this is supposed to
be a publish subject.

The Import/Export validation ensures that AllowTrace is used
by the correct type (service or stream)

Signed-off-by: Ivan Kozlovic <[email protected]>
@kozlovic
Copy link
Member Author

I have a server branch that references this branch to validate that the changes would work on the server: nats-io/nats-server@3608bcf

@coveralls
Copy link

coveralls commented Feb 14, 2024

Pull Request Test Coverage Report for Build 7919127075

Details

  • 0 of 27 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 81.042%

Totals Coverage Status
Change from base Build 7274328129: 0.1%
Covered Lines: 2723
Relevant Lines: 3360

💛 - Coveralls

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM!

@kozlovic
Copy link
Member Author

@aricart Could you review when you get a chance? And after review/updates, once it is merged, would it be possible to have a release (I guess v0.26.0 since fields were added)? I will then update the NATS Server branch that I have with proper go.mod dependency. Thanks!

@@ -229,6 +230,7 @@ type Account struct {
DefaultPermissions Permissions `json:"default_permissions,omitempty"`
Mappings Mapping `json:"mappings,omitempty"`
Authorization ExternalAuthorization `json:"authorization,omitempty"`
TraceDest Subject `json:"trace_dest,omitempty"`
Copy link
Member

@aricart aricart Feb 14, 2024

Choose a reason for hiding this comment

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

LGTM - but I wonder if we should have a container for this option in case there are other possible trace options. If it won't change, then possibly fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment there is nothing more, but I could see that we need to add things related to tracing indeed. What would you propose? Something like:

Trace              *MsgTrace             `json:"trace,omitempty"`
...
type MsgTrace struct {
	Destination Subject `json:"dest,omitempty"`
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes that would be perfect!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Please review changes, and as I mentioned previously, if you merge, could you please (or whoever is in charge) do a release so I can reference it in the NATS Server PR that I am working on? Thanks!

Copy link
Member

@aricart aricart left a comment

Choose a reason for hiding this comment

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

LGTM

@aricart aricart merged commit ed3fbfa into main Feb 15, 2024
6 checks passed
@kozlovic kozlovic deleted the add_msg_trace_support branch February 15, 2024 16:59
@aricart
Copy link
Member

aricart commented Feb 15, 2024

@kozlovic I released the library but since there were no incompatible changes, I just bumped micro.

@kozlovic
Copy link
Member Author

Thanks @aricart!

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