Adds new EventCounterCollectorModule #1224
Conversation
…rive and non specified intervals. but overall count and value should be same.
...CounterCollector.Tests/EventCounterCollector.Tests/EventCounterCollectorDiagnoticListener.cs
Outdated
Show resolved
Hide resolved
...EventCounterCollector/EventCounterCollector.Tests/EventCounterCollector.Tests/TestChannel.cs
Outdated
Show resolved
Hide resolved
Src/EventCounterCollector/EventCounterCollector/EventCounterCollectionModule.cs
Show resolved
Hide resolved
Src/EventCounterCollector/EventCounterCollector/EventCounterCollectionModule.cs
Outdated
Show resolved
Hide resolved
Src/EventCounterCollector/EventCounterCollector/EventCounterListener.cs
Outdated
Show resolved
Hide resolved
| var counterName = payload.Value.ToString(); | ||
| if (this.countersToCollect[eventSourceName].Contains(counterName)) | ||
| { | ||
| metricTelemetry.Name = string.Format(CultureInfo.InvariantCulture, "{0}|{1}", eventSourceName, counterName); |
There was a problem hiding this comment.
it is cheaper to concatenate strings than call string.format. We are on the hot path here
There was a problem hiding this comment.
Not sure what is the recommendation here..https://stackoverflow.com/questions/296978/when-is-it-better-to-use-string-format-vs-string-concatenation
I am unable to find concrete recommendation about one vs other.
| var counterName = payload.Value.ToString(); | ||
| if (this.countersToCollect[eventSourceName].Contains(counterName)) | ||
| { | ||
| metricTelemetry.Name = eventSourceName + "|" + counterName; |
There was a problem hiding this comment.
Is this what appears to the users? If so you should use the DisplayName property in the payload instead. Our guidance is that Name is the property that should be used internally within code, command-line parameters, etc. whereas DisplayName property is the one that gets shown on APM/dashboards/etc.
There was a problem hiding this comment.
Yes! good point. I somehow missed this. Will do a followup PR to address this and actually allow users to specific 'reportAs' as well, so users can decide what the name should appear.
...CounterCollector.Tests/EventCounterCollector.Tests/EventCounterCollectorDiagnoticListener.cs
Outdated
Show resolved
Hide resolved
Src/EventCounterCollector/EventCounterCollector/EventCounterListener.cs
Outdated
Show resolved
Hide resolved
|
Other than the two comments I made above I think things look fine (from a quick glance) - Please feel free to tag me on any follow-up PRs regarding counters/EventListener and I'd be happy to help answer any questions :-) |
Fix Issue #1222
Adds new Nuget package, new ITelemetryModule for collecting EventCounters.
I am working on adding Unit Tests, please review product code.
For significant contributions please make sure you have completed the following items:
Changes in public surface reviewed
Design discussion issue #
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.
The PR will trigger build, unit tests, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.
If you want to to re-run the build/tests, the easiest way is to simply Close and Re-Open this same PR. (Just click 'close pull request' followed by 'open pull request' buttons at the bottom of the PR)
Please follow [these] (https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/develop/CONTRIBUTING.md) instructions to build and test locally.