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.

Sign up for GitHub

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

Jump to bottom

Add support for System.Diagnostics.Metrics in dotnet-monitor's collector #3587

Merged
merged 24 commits into from
Jan 30, 2023
Merged

Add support for System.Diagnostics.Metrics in dotnet-monitor's collector #3587

merged 24 commits into from
Jan 30, 2023

Conversation

wiktork
Copy link
Member

@wiktork wiktork commented Jan 4, 2023

@kkeirstead completed most of code here.

Note: Currently the code duplicates the data model from the original dotnet-counters work.

Note there is additional work that is built on top of this feature branch

  • Unification between dotnet-monitor and dotnet-counters
  • Multiplexing to support multiple simultaneous System.Diagnostics.Metrics sessions.

kkeirstead and others added 9 commits November 30, 2022 13:15
@kkeirstead @dotnet-maestro @wiktork
[Dotnet Monitor] Ignore - Adding System.Diagnostics.Metrics Support (#…
d55df48 
…3529)

* Got basic counter rate end-to-end working - currently in a broken state as I investigate other types of metrics

* Leftovers from previous commit

* Gauges working for systems diagnostics metrics

* Added in histogram, adding in options for maxHistograms and maxTimeSeries

* Added in error payloads for logging purposes

* Temporarily changed visibility for testing - this may be reverted later

* Now handling multiple sessions

* Added filtering for counters, instead of allowing all counters for a provider to go through.

* Handle observable... errors

* Some cleanup, added error event check

* Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20221129.1 (#3528)

[main] Update dependencies from dotnet/source-build-reference-packages

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
@kkeirstead
[Dotnet Monitor] Revert To Logging a single payload (not a list) (#3538)
e6b5f21
@wiktork
Merge pull request #3545 from dotnet/main
b52b4c4 
Merge main to feature branch
@wiktork
[Feature branch changes, no review] Fixup counter apis (#3559)
ddf091e 
* Fix ICountersLogger contract

* MetadataUpdates

* Fixup api protection levels

* Fixup tests and add CounterEnded payload

* Fixup metadata parsing
@kkeirstead
[Dotnet-Monitor] [Feature Branch] Switched quantile to Percentile (#3567
959ebd6 
)
@wiktork
Merge pull request #3575 from dotnet/main
f3fd8a6 
Merge main to feature/eventPipeMeters
@wiktork
Merge pull request #3579 from dotnet/main
c822924 
merge main to feature/eventPipeMeters
@wiktork
Minor branch cleanup
5052752
@wiktork
Merge pull request #3580 from wiktork/dev/wiktork/cleanupFeature
1b70d14 
Minor branch cleanup
@wiktork wiktork requested review from kkeirstead and noahfalk January 4, 2023 18:25
@wiktork wiktork changed the title Add support for System.Diagnostics.Metrics id dotnet-monitor's collector Add support for System.Diagnostics.Metrics in dotnet-monitor's collector Jan 4, 2023
noahfalk
noahfalk reviewed Jan 5, 2023
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

This seems fine so far to me!

src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/EventCounterPipeline.cs Show resolved Hide resolved
@noahfalk
Copy link
Member

noahfalk commented Jan 5, 2023

Sweet this is one of the big gaps with the new Metrics API that I wanted to get addressed and I was going to come talk to you all about it. I'm very happy to see you beat me to it!

@wiktork wiktork mentioned this pull request Jan 13, 2023
Merged
@wiktork wiktork marked this pull request as ready for review January 17, 2023 16:55
@wiktork wiktork requested a review from a team as a code owner January 17, 2023 16:55
noahfalk
noahfalk approved these changes Jan 18, 2023
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM modulo a few inline comments

src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs Outdated Show resolved Hide resolved
@wiktork wiktork mentioned this pull request Jan 20, 2023
Merged
@wiktork wiktork mentioned this pull request Jan 26, 2023
Merged
wiktork
wiktork commented Jan 27, 2023
View reviewed changes
src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs Outdated

if (commaIndex < 0)
if (string.IsNullOrEmpty(quantilesText))
Copy link
Member Author

Choose a reason for hiding this comment

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

@noahfalk after histogram values are no longer reported regularly in the app, the quantiles here become empty. is this by design?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, the app hasn't disposed the Histogram and also hasn't recorded any data within the measurement interval then by design you get an event where the quantiles string is empty. There is a test case for that behavior here: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs#L374

When doing the design initially I thought that sending an explicit event with empty content would let tools know with confidence that the app didn't produce data whereas if an event is absent it is ambiguous what the app did.

Copy link
Member

Choose a reason for hiding this comment

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

Should this then return a CounterEndedPayload instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll double check this behavior, but I believe it produces more data if more data occurs. I will change our pipeline to produce the event with no quantiles, and we can simply discard them during consumption.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the MetricEventSource events should behave the way you are describing Wiktor. You could choose to model it different ways when you are processing it but looking at Prometheus probably provides a good point of comparison. You could make a Prometheus.NET histogram, record no data, and see what shows up on the metrics endpoint.

jander-msft
jander-msft reviewed Jan 27, 2023
View reviewed changes
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Configuration/MetricSourceConfiguration.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs
}

// Copied from dotnet-counters
public CounterPayload(string providerName, string name, string displayName, string displayUnits, string metadata, double value, DateTime timestamp, string type, EventType eventType)
Copy link
Member

Choose a reason for hiding this comment

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

displayName is not used; either remove it, or (preferably) have it set the DisplayName property, change the property to get-only, and call all upstreams to pass in the coerced display name value.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a separate change for cleaning up all these constructors.

src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPipelineSettings.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs Outdated Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs Outdated

if (commaIndex < 0)
if (string.IsNullOrEmpty(quantilesText))
Copy link
Member

Choose a reason for hiding this comment

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

Should this then return a CounterEndedPayload instance?

src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/TraceEventExtensions.cs Show resolved Hide resolved
src/Microsoft.Diagnostics.Monitoring.EventPipe/Counters/CounterPayload.cs Outdated Show resolved Hide resolved
@wiktork wiktork merged commit 615b21d into main Jan 30, 2023
@hoyosjs hoyosjs deleted the feature/eventPipeMeters branch January 31, 2023 19:14
@hoyosjs hoyosjs restored the feature/eventPipeMeters branch January 31, 2023 19:14
@hoyosjs hoyosjs mentioned this pull request Feb 1, 2023
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kkeirstead kkeirstead kkeirstead left review comments

@jander-msft jander-msft jander-msft left review comments

@noahfalk noahfalk noahfalk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wiktork @noahfalk @jander-msft @kkeirstead