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

Limit metric tag to concrete message type #7057

Merged
merged 5 commits into from
Jun 17, 2024

Conversation

lailabougria
Copy link
Contributor

@lailabougria lailabougria commented Jun 4, 2024

related to #7022

Currently, a metric tag containing the content of the EnclosedMessageTypes header is added to the Fetches, Failures, and Success metrics emitted through System.Diagnostics in Core.

However, the value of this header can be quite large, and doesn't provide as much value, as filtering makes most sense on the message type, not its entire hierarchy tree.
This PR changes that value to be limited to the most concrete message type, and only includes the message type full name, and excludes assembly info.

The full contents of the header are still emitted as trace tags.

Copy link
Member

@SzymonPobiega SzymonPobiega left a comment

Choose a reason for hiding this comment

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

Nitpicking ;)

Co-authored-by: Szymon Pobiega <szymon.pobiega@gmail.com>
@SzymonPobiega SzymonPobiega merged commit fd29208 into master Jun 17, 2024
3 checks passed
@SzymonPobiega SzymonPobiega deleted the limit-metric-tag-to-concrete-handlertype branch June 17, 2024 07:56
@@ -18,12 +19,14 @@ public ReceiveDiagnosticsBehavior(string queueNameBase, string discriminator)
public async Task Invoke(IIncomingPhysicalMessageContext context, Func<IIncomingPhysicalMessageContext, Task> next)
{
context.MessageHeaders.TryGetValue(Headers.EnclosedMessageTypes, out var messageTypes);
var messageTypeHeader = !string.IsNullOrEmpty(messageTypes) ? messageTypes.Split(';').FirstOrDefault() : default;
Copy link

Choose a reason for hiding this comment

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

Would we ever want to handle a weirdly formatted message type string that starts with a ;. Maybe this is too edge and we'd want to fix the thing creating the buggy data instead.

using System;
using System.Linq;

namespace HelloWorld
{
	public class Program
	{
		public static void Main(string[] args)
		{
		  var messageTypes = ";ClassName1,unecessary information;ClassName2,unecessary information";
			var messageTypeHeader = !string.IsNullOrEmpty(messageTypes) ? messageTypes.Split(';').FirstOrDefault(s => !string.IsNullOrEmpty(s)) : null;
			var messageTypeName = !string.IsNullOrEmpty(messageTypeHeader) ? messageTypeHeader.Split(',').FirstOrDefault(s => !string.IsNullOrEmpty(s)) : null;
			
			Console.WriteLine(messageTypeName);
		}
	}
}

Copy link

Choose a reason for hiding this comment

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

@lailabougria Usually, we like to cache these types of string manipulations in a ConcurrentDictionary for performance reasons?

messageTypes = enclosedMessageTypesStringToMessageTypes.GetOrAdd(enclosedMessageTypesValue,

https://github.com/Eventellect/SupportRepro/blob/39b6264282688f21737990014e2b5b72faa4b660/src/NSB.Extensions/EnclosedMessageTypesMappingBehavior.cs#L59

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbrandt Thanks for pointing out the caching optimization; we'll look into this and, if needed, raise a follow-up PR.

Regarding your first question, we wouldn't handle that scenario because we assume this input was either:

  • created by another nservicebus endpoint
  • created by a user in native-integration scenarios, which would never work with that type of input anyway

If it were to happen that the header is formatted as you suggested, then it would not fail in this code, but rather have an empty value inside the metric tag. However, it would later fail in deserializing the message, and cause message processing to fail (which you would see reflected in traces).

@SzymonPobiega SzymonPobiega added this to the 9.1.0 milestone Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants