Msmq integration by anna-git · Pull Request #1457 · DataDog/dd-trace-dotnet

anna-git · 2021-05-10T13:39:59Z

Fixes #

Changes proposed in this pull request:

3 intercepted methods
integration test
sample program

@DataDog/apm-dotnet

needs some more checks

andrewlock

Looking good! 🙂 I left a few comments mostly around whether we want to include all of the queue details as tags for performance reasons

andrewlock · 2021-05-10T15:00:11Z

+        public override string SpanKind { get; }
+
+        public string Queue { get; set; }
+
+        public string QueueLabel { get; set; }
+
+        public string QueueLastModifiedTime { get; set; }
+
+        public string UniqueQueueName { get; set; }
+
+        public string TransactionType { get; set; }
+
+        public string IsTransactionalQueue { get; set; }
+
+        public string InstrumentationName { get; set; }


This is a lot of tags, do we definitely want them all? I've learnt (the hard way!) that each tag has a significant performance impact! IMO, we could probably lose:

QueueLabel

QueueLastModifiedTime

IsTransactionalQueue

UniqueQueueName (i.e. either keep formatName or queueName, depending on which is more useful!)

TransactionType

Additionally InstrumentationName is normally created as a readonly field in the Tags, rather than specified at creation 🙂

Tacking onto this, as a user of MSMQ, I would find these types of things useful:

Queue name

Path

Address

As they would allow me to identify the queue, and any other information can be uncovered by knowing the unique queue.

Oh ok good to know! will remove those and keep format which gives the host info.

In msmq, if we send a message within a transaction to a non transactional queue, or the opposite, sending a message outside of a transaction context to a transactional queue, the message is never sent but the methods are called and nothing crashes, which is kind of an odd behavior I got stuck on. So I thought it might be useful to a customer to see, in the operation msmq.send, for example:

is_transactional_queue > False
transaction_type> Single (there is Auto, None, Single)

but it's just a remark, I'm not sure of their requirements here :)

Tacking onto this, as a user of MSMQ, I would find these types of things useful:

Queue name

Path

Address

As they would allow me to identify the queue, and any other information can be uncovered by knowing the unique queue.

@colin-higgins I dont see any Address property, you mean MulticastAddress property only available from from Msmq 3.0 ?

Otherwise queue and path might be enough?
for ex
Path: .\Private$\private-nontransactional-queue
Unique name : DIRECT=OS:desktop-duevvft\Private$\private-nontransactional-queue

If unique name includes Path (as it seems to from that example), then that will probably be sufficient IMO. (Spoken as someone that has never used MSMQ!)

andrewlock · 2021-05-10T15:00:38Z

+        /// Initializes a new instance of the <see cref="MsmqTags"/> class.
+        /// </summary>
+        /// <param name="spanKind">kind of span</param>
+        public MsmqTags(string spanKind) => SpanKind = spanKind;


You'll need to create a default constructor version of this too, as there's a unit tests that checks for its existence

andrewlock · 2021-05-10T15:03:26Z

+
+        private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor(typeof(MsmqCommon));
+
+        internal static Scope CreateScope(Tracer tracer, string command, string spanKind, string queueName, string formatName, string queueLabel, DateTime queueLastModifiedTime, bool withinTransaction, string transactionType, bool transactionalQueue, out MsmqTags tags)


This is a matter of preference, but could we pass an IMessageQueue in here instead of the strings? Also I don't think the out parameter is used anywhere?

andrewlock · 2021-05-10T15:04:55Z

+        /// Returns:
+        ///   true to create and use a connection cache; otherwise, false.
+        /// </summary>
+        bool EnableConnectionCache { get; }


I don't think all of these properties are used in the integration? We have to generate code for each extra property, so AFAIK it's best to only specify the properties you actually need

- constructor msmqtags - remove some properties - transactional messages and queue, properties make more sense - unused params

kevingosse

Great stuff 👍 Only minor changes needed

kevingosse · 2021-05-12T09:36:09Z

      }
    ]
  },
+  {


Please remember to re-generate this file before merging, as we are in version 1.26.3 now

kevingosse · 2021-05-12T09:40:18Z

+
+        public const string MsmqCursorHandle = "System.Messaging.Interop.CursorHandle";
+        public const string MsmqMessageQueueTransaction = "System.Messaging.MessageQueueTransaction";
+        public const string MsmqMessageQueueTransactionType = "System.Messaging.MessageQueueTransactionType";
+        public const string MsmqMessagePropertyFilter = "System.Messaging.MessagePropertyFilter";
+        public const string MsmqMessage = "System.Messaging.Message";


As much as possible, we should put in ClrNames only base types that may be useful for other integrations. For the MSMQ ones, you should declare them in the same folder as the integration (for instance in MsmqConstants)

kevingosse · 2021-05-12T09:45:03Z

+        private const string CommandPeek = "msmq.peek";
+        private const string CommandConsume = "msmq.consume";
+        private const string IntegrationName = nameof(IntegrationIds.Msmq);


NIT: you could move them to MsmqConstants (same for similar constants in other integration files)

kevingosse · 2021-05-12T09:51:53Z

        AspNetMvc,
        AspNetWebApi2,
        GraphQL,
+        Msmq,


I don't think it would cause any version conflict, but I'd feel safer if you declared it last, so that the numerical id of the existing integrations don't change

kevingosse · 2021-05-12T09:53:02Z

+
+        public string InstrumentationName => "msmq";
+
+        public string IsTransactionalQueue { get; set; }


I wonder if metrics would be a better storage than string for boolean value... I don't know if we have booleans elsewhere. @lucaspimentel thoughts?

What's the purpose of this value? If we want to display it in the UI, it needs to be a span tag. We don't display span "metrics" in the UI.

kevingosse · 2021-05-12T09:55:33Z

+{
+    public class Program
+    {
+        const string PrivateTransactionalQueuePath = ".\\Private$\\private-transactional-queue";


I'm curious to see if that will work in the CI agent. That would be a pleasant surprise if the MSMQ service is activated.

tonyredondo

Looking good so far!, I've just left a couple of comments performance related.

tonyredondo · 2021-05-12T10:19:45Z

+
+        private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor(typeof(MsmqCommon));
+
+        internal static Scope CreateScope(Tracer tracer, string command, string spanKind, IMessageQueue messageQueue, bool? messagePartofTransaction = null)


If we use IMessageQueue as parameter then the proxy struct will be boxed. You can use the generic constraint as in the integration.

lucaspimentel · 2021-05-12T14:19:43Z

+{
+    internal class MsmqCommon
+    {
+        internal const string IntegrationName = nameof(IntegrationIds.Msmq);


Can you reuse the value in MsmqConstants?

lucaspimentel · 2021-05-12T14:20:21Z

+    public class MessageQueue_Purge_Integration
+    {
+        private const string Command = "msmq.purge";
+        private const string IntegrationName = nameof(IntegrationIds.Msmq);


Can you reuse the value in MsmqConstants.IntegrationName? Consider also moving "msmq.purge" there.

lucaspimentel · 2021-05-12T14:20:45Z

+    IntegrationName = IntegrationName)]
+    public class MessageQueue_SendInternal_Integration
+    {
+        private const string IntegrationName = nameof(IntegrationIds.Msmq);


Can you reuse the value in MsmqConstants?

lucaspimentel · 2021-05-12T15:41:42Z

+
+                var span = scope.Span;
+                span.Type = SpanTypes.Queue;
+                span.ResourceName = command;


What will the resource name look like? I tried looking in the tests and we're not asserting that there (we probably should).

If users are sending messages to different queues, we want those grouped by queue name in the UI. Should we append the Path to the current resource name?. Something like {command} {path}. Similar to http requests which use {verb} {path} (e.g. GET /users/edit/{id}).

lucaspimentel · 2021-05-12T17:06:33Z

@anna-git, can we close this PR in favor of #1463?

anna-git · 2021-05-13T12:19:23Z

@anna-git, can we close this PR in favor of #1463?

@lucaspimentel yes please thanks :)

) * msmq integration Co-authored-by: anna-git <anna,[email protected]>

anna-git and others added 17 commits May 4, 2021 16:19

3 integrations for msmq: purge, receivecurrent, sendinternal

d1d675b

Send internal integration completed

4d0207a

receive hook > null message

a2347c5

Debuggin receive current integration, message being null

669775a

3 integrations methods ready

edb2154

integrations tests start to pass.

91c3076

needs some more checks

3 integrations for msmq: purge, receivecurrent, sendinternal

8b77f80

Send internal integration completed

ba1ecd7

receive hook > null message

24f7ae6

Debuggin receive current integration, message being null

552423d

3 integrations methods ready

84a9f00

integrations tests start to pass.

da72183

needs some more checks

adding more checks in integration tests

bd6526b

Merged after rebase

f804087

Remove useless interface

31528c9

added some documentation

81963fe

renaming an msmq clr name

deb0348

anna-git requested a review from a team as a code owner May 10, 2021 13:39

andrewlock reviewed May 10, 2021

View reviewed changes

anna-git added 2 commits May 10, 2021 17:12

fixed type name tests, added 5 types

3bd43a4

fixed:

6b2bb5c

- constructor msmqtags - remove some properties - transactional messages and queue, properties make more sense - unused params

kevingosse suggested changes May 12, 2021

View reviewed changes

tonyredondo requested changes May 12, 2021

View reviewed changes

lucaspimentel reviewed May 12, 2021

View reviewed changes

anna-git pushed a commit that referenced this pull request May 12, 2021

Msmq integration, taking into account all comments from PR #1457.

59cdeb3

anna-git mentioned this pull request May 12, 2021

Msmq integration, taking into account all comments from PR #1457. #1463

Merged

lucaspimentel added area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) type:new-feature labels May 12, 2021

zacharycmontoya mentioned this pull request May 12, 2021

Add kafka instrumentation #1444

Merged

anna-git pushed a commit that referenced this pull request May 14, 2021

Msmq integration, taking into account all comments from PR #1457.

77207b5

anna-git closed this May 14, 2021

anna-git added a commit that referenced this pull request May 17, 2021

Msmq integration, taking into account all comments from PR #1457. (#1463

f822b94

) * msmq integration Co-authored-by: anna-git <anna,[email protected]>


		private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor(typeof(MsmqCommon));

		internal static Scope CreateScope(Tracer tracer, string command, string spanKind, string queueName, string formatName, string queueLabel, DateTime queueLastModifiedTime, bool withinTransaction, string transactionType, bool transactionalQueue, out MsmqTags tags)


		public string InstrumentationName => "msmq";

		public string IsTransactionalQueue { get; set; }

Uh oh!

Conversation

anna-git commented May 10, 2021

Uh oh!

andrewlock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevingosse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucaspimentel May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonyredondo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucaspimentel May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucaspimentel commented May 12, 2021

Uh oh!

anna-git commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lucaspimentel May 12, 2021 •

edited

Loading

lucaspimentel May 12, 2021 •

edited

Loading

anna-git commented May 13, 2021 •

edited

Loading