Skip to content

Allows linking nodes in the message flow view as a fallback when no link could be established on OriginatingEndpoint / ProcessingEndpoint mismatch #1171

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

Merged

Conversation

ramonsmits
Copy link
Member

@ramonsmits ramonsmits commented Feb 9, 2022

In order to link nodes the following must be true:

child.RelatedTo == parent.Id && child.OriginatingEndpoint == parent.ProcessingEndpoint

However, in situaties where only processed messages can be linked based on RelatedTo these messages remain unlinked in the message flow view.

Before:

image

After:

image

Logic is written in such a way that the fallback only happens if intent not equal to publish. Meaning, only messages and commands as these SHOULD only have one processing endpoint

Implications

This is a fallback, the default linking behavior remains the same is the message flow is purely passing only NServiceBus endpoints but adds linking when that is not always the case (i.e. native integration).

@ramonsmits ramonsmits requested a review from WilliamBZA February 9, 2022 17:10
@ramonsmits ramonsmits self-assigned this Feb 10, 2022
Copy link
Member

@WilliamBZA WilliamBZA left a comment

Choose a reason for hiding this comment

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

Do you think it's possible to add a unit test for this?

@ramonsmits
Copy link
Member Author

Do you think it's possible to add a unit test for this?

@WilliamBZA No tests exist at all against this view model. Potentially the LinkConversationNodes method could be extracted and unit tested as the input/output is a conversion.

@ramonsmits ramonsmits requested a review from WilliamBZA March 9, 2022 12:37
@WilliamBZA
Copy link
Member

I'm not sure that I'm the right person to review this PR. I can review the code but am not really sure I'm able to follow the implication of the change.

Copy link
Contributor

@danielmarbach danielmarbach left a comment

Choose a reason for hiding this comment

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

I don't have enough context to argue whether we should proceed with this. I did try to provide some nitpicks though :trollface:

@HEskandari
Copy link
Contributor

@danielmarbach to answer your question: logs are displayed within SI in the Logs window. The reason to mark them as Warn/Error was to make them stand out as we log other things there as well. If you want to argue "what's the action" you can argue why we log anything at all.

The error logs in the diagram code are not 'expected'. Maybe those can be replaced by exceptions. Those conditions are not expected to happen. When customers do report those, we'll need to check their (SC) data to see why that happens.

@stale stale bot added the stale label Apr 27, 2022
@ramonsmits ramonsmits removed the stale label May 2, 2022
@Particular Particular deleted a comment from stale bot May 2, 2022
@stale stale bot added the stale label Jul 7, 2022
@Particular Particular deleted a comment from stale bot Jul 8, 2022
@stale stale bot removed the stale label Jul 8, 2022
@stale stale bot added the stale label Aug 10, 2022
@Particular Particular deleted a comment from stale bot Aug 10, 2022
@stale stale bot removed the stale label Aug 10, 2022
@stale stale bot added the stale label Sep 16, 2022
@Particular Particular deleted a comment from stale bot Sep 22, 2022
@ramonsmits ramonsmits removed the stale label Sep 22, 2022
@github-actions github-actions bot added the stale label Oct 23, 2022
@Particular Particular deleted a comment from github-actions bot Oct 24, 2022
@github-actions github-actions bot removed the stale label Oct 25, 2022
@github-actions github-actions bot added the stale label Nov 25, 2022
@ramonsmits ramonsmits force-pushed the relatedto-endpoint-mismatch-fallback branch from 9147a3c to 8e77754 Compare November 25, 2022 08:02
@Particular Particular deleted a comment from github-actions bot Nov 25, 2022
@ramonsmits ramonsmits changed the title Allows linking nodes in the message flow view on child.OriginatingEndpoint = parent.ProcessingEndpoint mismatch Allows linking nodes in the message flow view as a fallback when no link could be established on child.OriginatingEndpoint = parent.ProcessingEndpoint mismatch Nov 25, 2022
@ramonsmits ramonsmits changed the title Allows linking nodes in the message flow view as a fallback when no link could be established on child.OriginatingEndpoint = parent.ProcessingEndpoint mismatch Allows linking nodes in the message flow view as a fallback when no link could be established on OriginatingEndpoint / ProcessingEndpoint mismatch Nov 25, 2022
@ramonsmits ramonsmits added this to the 2.11.0 milestone Nov 25, 2022
@ramonsmits ramonsmits force-pushed the relatedto-endpoint-mismatch-fallback branch from c0d5e85 to c0dc8fa Compare November 25, 2022 10:32
@ramonsmits ramonsmits changed the base branch from master to dev-exp-upgrade-buid-fix November 25, 2022 10:32
@ramonsmits ramonsmits marked this pull request as draft November 25, 2022 10:41
@ramonsmits
Copy link
Member Author

Is rebased on top of dev-exp-upgrade-buid-fix, needs #1288 to be merged first.

@github-actions github-actions bot removed the stale label Nov 26, 2022
@ramonsmits ramonsmits marked this pull request as ready for review November 29, 2022 11:55
@HEskandari HEskandari merged commit bdc5d8f into dev-exp-upgrade-buid-fix Nov 29, 2022
@HEskandari HEskandari deleted the relatedto-endpoint-mismatch-fallback branch November 29, 2022 22:45
cquirosj added a commit that referenced this pull request Dec 6, 2022
…ink could be established on OriginatingEndpoint / ProcessingEndpoint mismatch (#1171)

* Allows linking nodes if originating message is not an event in case there is no match on child.RelatedTo = parent.Id && child.OriginatingEndpoint = parent.ProcessingEndpoint

* Implemented logging TODO's

* Removed unneeded TODO

* Resolved ESSENTIAL formatting fix IDE0055......

* Apply suggestions from code review

Co-authored-by: Daniel Marbach <[email protected]>

* Update src/ServiceInsight/MessageFlow/MessageFlowViewModel.cs

* Using names instead of numbers for Serilog messages

* Fix missing ToArray

* Converted to switch for reabability of intent

Co-authored-by: Daniel Marbach <[email protected]>
# Conflicts:
#	src/ServiceInsight/MessageFlow/MessageFlowViewModel.cs
cquirosj added a commit that referenced this pull request Dec 6, 2022
…ink could be established on OriginatingEndpoint / ProcessingEndpoint mismatch (#1171)

* Allows linking nodes if originating message is not an event in case there is no match on child.RelatedTo = parent.Id && child.OriginatingEndpoint = parent.ProcessingEndpoint

* Implemented logging TODO's

* Removed unneeded TODO

* Resolved ESSENTIAL formatting fix IDE0055......

* Apply suggestions from code review

Co-authored-by: Daniel Marbach <[email protected]>

* Update src/ServiceInsight/MessageFlow/MessageFlowViewModel.cs

* Using names instead of numbers for Serilog messages

* Fix missing ToArray

* Converted to switch for reabability of intent

Co-authored-by: Daniel Marbach <[email protected]>
# Conflicts:
#	src/ServiceInsight/MessageFlow/MessageFlowViewModel.cs
soujay pushed a commit that referenced this pull request Dec 6, 2022
…ink could be established on OriginatingEndpoint / ProcessingEndpoint mismatch (#1171) (#1294)

* Allows linking nodes if originating message is not an event in case there is no match on child.RelatedTo = parent.Id && child.OriginatingEndpoint = parent.ProcessingEndpoint

* Implemented logging TODO's

* Removed unneeded TODO

* Resolved ESSENTIAL formatting fix IDE0055......

* Apply suggestions from code review

Co-authored-by: Daniel Marbach <[email protected]>

* Update src/ServiceInsight/MessageFlow/MessageFlowViewModel.cs

* Using names instead of numbers for Serilog messages

* Fix missing ToArray

* Converted to switch for reabability of intent

Co-authored-by: Daniel Marbach <[email protected]>
# Conflicts:
#	src/ServiceInsight/MessageFlow/MessageFlowViewModel.cs
@soujay soujay removed this from the 2.11.0 milestone Dec 6, 2022
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.

Nodes in the message flow view are not linked if the OriginatingEndpoint and ProcessingEndpoint do not match
6 participants