-
Notifications
You must be signed in to change notification settings - Fork 649
Add the ability to generate manifest info for an endpoint in diagnostics #7416
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
Conversation
…ceBus into manifest_spike
…ing all items that belong in the manifest
| type => new | ||
| { | ||
| MessageType = type, | ||
| IsMessage = conventions.IsMessageType(type), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this add value? We are iterating over the known message types, so this would always be true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was pre-existing in AutoSubscribe, I just moved and refactored the code
| } | ||
|
|
||
| config.Settings.Set(new AuditConfigReader.Result(auditQueue, timeToBeReceived)); | ||
| config.Settings.AddStartupDiagnosticsSection("Manifest-AuditQueue", auditQueue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to introduce the concept of a "category" for a section? Essentially, currently we are using "category-" as a convention to say these are all manifest related sections. Today the feature diagnostic code does a distinct on the section names and warns for duplicated entries. If we add an optional "category" (or whatever else makes sense) we could augment the writing code to move stuff of the same category into a subsection. That would allow us to also leverage this, for example, in the persistence or transport to have a category "Persistence" or "Transport" and then have all the related sections grouped under there. This means it is no longer strictly necessary to have unique names like "NServiceBus.Persistence.Sql.Outbox" but we could standardize on having a persistence category with sections like Outbox, Sagas, Subscriptions (per storage type) or in mixed scenarios keep "Persistence" as a category and have "SqlOutbox", "SqlSagas" and "RavenSubscriptions" (just making up examples for now for the discussion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking along the same lines, but since the diagnostics order the output alphabetically the "Manifest-" achieved the same thing without having to force a new standard on every diagnostic call
| IsMessage = handledMessage.IsMessage, | ||
| IsEvent = handledMessage.IsEvent, | ||
| IsCommand = handledMessage.IsCommand, | ||
| Schema = handledMessage.MessageType.GetProperties().Select( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not necessarily be a problem we have to solve now, but it is worth discussing. More reflection done at startup will increase the startup time. The other PR made the manifest writing opt-in. Diagnostics are currently something that is always written (which has pros and cons). That being said, even the original spike also did the "compute intensive" part as part of the startup, independent of whether the manifest was enabled. So the concern I raise here is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair call, but that would require re-adding in an EnableManifest api, either experimental or otherwise. This isn't the only place that reflection is used as part of startup, but I conceded the point that it will be adding to the already significant burden.
As an aside, wouldn't the assembly scan be the slowest part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed, pass Func<object> rather than object for expensive operations, and enable these to be filtered out by the diagnostics writer should startup performance be an issue
|
design document for the manifest implementation |
alternative to #7406
This is the first step for Ability to export resources manifest required for endpoints deployment
To start with, the ASB transport and the SqlPersistence have had manifest generation implementations added.
sample output with:
"SubscribedEventToTopicsMap": {"Shared.EventOne": [ "event-one", "event-bla" ],"Shared.EventTwo": [ "event-bla" ]