Msmq integration, taking into account all comments from PR #1457.#1463
Conversation
lucaspimentel
added
area:automatic-instrumentation
|
did a rebase to get the fix for the build |
| { | ||
| Command = command, | ||
| IsTransactionalQueue = messageQueue.Transactional.ToString(), | ||
| Path = messageQueue.Path, |
There was a problem hiding this comment.
For the Path tag (not the resource name), I think it would be desirable to include the machine name, that way users can filter further on that. Is that reasonable @lucaspimentel or should we not include the machine name on this tag?
There was a problem hiding this comment.
The machine name property doesn't give me the machine name but just a "." so does seem useless..
The only way to get it is to use the format name property of the queue. So path and format name look almost the same..
Path: Private$\myQueue3
FormatName: DIRECT=OS:desktop-duevvft\Private$\myQueue3
we need the path for grouping by resource name:
span.ResourceName = $"{command} {messageQueue.Path}";
I can try to isolate the machine name by removing the "Path" part from the format name, it should cover all sorts of cases?
There was a problem hiding this comment.
Got it. With that info, then I would recommend keeping what you have now so we don't spend extra cycles splitting and allocating new strings 😄
zacharycmontoya
left a comment
zacharycmontoya
left a comment
There was a problem hiding this comment.
LGTM with one small question about a tag's value 👍🏼
|
|
||
| namespace Datadog.Trace.ClrProfiler.AutoInstrumentation.Msmq | ||
| { | ||
| internal class MsmqCommon |
6 participants
Fixes #
Changes proposed in this pull request:
Taken into accounts all comments from #1457 because I couldn't rebase successfully despite all attempts
@DataDog/apm-dotnet