Skip to content

Use a singular option for all counter intervals #944

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
Merged
merged 6 commits into from
Oct 3, 2021
Merged

Use a singular option for all counter intervals #944

merged 6 commits into from
Oct 3, 2021

Conversation

@wiktork
Copy link
Member

@wiktork wiktork commented Oct 1, 2021 •
edited
Loading

  • Prometheus metrics
  • EventCounter trigger
  • Trace
  • Custom trace
  • Live metrics
  • Custom live metrics
  • trace action
  • custom trace action

Not done:

  • Asp.net duration heartbeat. Hardcoded in diagnostics repo
  • Make sure unit tests work
  • Documentation changes

@wiktork wiktork requested review from a team and IEvangelist as code owners October 1, 2021 22:47
@wiktork wiktork marked this pull request as draft October 1, 2021 22:47
@wiktork wiktork linked an issue Oct 1, 2021 that may be closed by this pull request
Metric based triggers interfere with /metrics #923
Closed
wiktork
wiktork commented Oct 1, 2021
View reviewed changes
wiktork
wiktork commented Oct 1, 2021
View reviewed changes
@wiktork wiktork marked this pull request as ready for review October 2, 2021 00:20
@wiktork wiktork force-pushed the dev/wiktork/interval branch from 5279086 to 3517d35 Compare October 2, 2021 00:22
wiktork
wiktork commented Oct 2, 2021
View reviewed changes
src/Tests/Microsoft.Diagnostics.Monitoring.Tool.UnitTests/CollectionRuleOptionsTests.cs
const double ExpectedGreaterThan = 0.5;
const double ExpectedLessThan = 0.75;
TimeSpan ExpectedDuration = TimeSpan.FromSeconds(30);
const int ExpectedFrequency = 5;
Copy link
Member Author

@wiktork wiktork Oct 2, 2021

Choose a reason for hiding this comment

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

Note that metricsRefreshInterval is still in the test api, but it's simply ignored.

Copy link
Contributor

@jander-msft jander-msft Oct 2, 2021

Choose a reason for hiding this comment

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

That can be fixed after these are merged.

jander-msft
jander-msft reviewed Oct 2, 2021
View reviewed changes
src/Microsoft.Diagnostics.Monitoring.Options/GlobalCounterOptions.cs

internal static class GlobalCounterOptionsExtensions
{
public static int GetIntervalSeconds(this GlobalCounterOptions options) =>
Copy link
Contributor

@jander-msft jander-msft Oct 2, 2021

Choose a reason for hiding this comment

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

I like this better than our current scattering usage of GetValueOrDefault everywhere. Separately, I think we should do this with all of the nullable options properties.

@jander-msft
Copy link
Contributor

jander-msft commented Oct 2, 2021

The only hesitation that I have about this change is that it is using IOptionsMonitor<T>, which allows the configuration to reflect updates from the binding sources. This means that the GlobalCounter:IntervalSeconds can be updated at any time and the new value will be used for new operations. Post preview, I think we need to consider caching the value per process when accessing it the first time for each process; this allows consistency throughout the lifetime of the process.

@wiktork
Copy link
Member Author

wiktork commented Oct 3, 2021

The only hesitation that I have about this change is that it is using IOptionsMonitor<T>, which allows the configuration to reflect updates from the binding sources. This means that the GlobalCounter:IntervalSeconds can be updated at any time and the new value will be used for new operations. Post preview, I think we need to consider caching the value per process when accessing it the first time for each process; this allows consistency throughout the lifetime of the process.

This is a reasonable concern. I think we can also address this by making sure things that depend on the interval properly reset their state when the configuration changes.

@wiktork wiktork force-pushed the dev/wiktork/interval branch from 3517d35 to 9ac77bc Compare October 3, 2021 05:18
@jander-msft
Copy link
Contributor

jander-msft commented Oct 3, 2021

The only hesitation that I have about this change is that it is using IOptionsMonitor<T>, which allows the configuration to reflect updates from the binding sources. This means that the GlobalCounter:IntervalSeconds can be updated at any time and the new value will be used for new operations. Post preview, I think we need to consider caching the value per process when accessing it the first time for each process; this allows consistency throughout the lifetime of the process.

This is a reasonable concern. I think we can also address this by making sure things that depend on the interval properly reset their state when the configuration changes.

I think the least acceptable scenario with this type of behavior would be cancelling a trace from the /trace route. But I'm not necessarily opposed to it.

jander-msft
jander-msft approved these changes Oct 3, 2021
View reviewed changes
@wiktork wiktork merged commit d80ebf0 into dotnet:main Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kkeirstead kkeirstead kkeirstead left review comments

@kelltrick kelltrick Awaiting requested review from kelltrick

@IEvangelist IEvangelist Awaiting requested review from IEvangelist

+1 more reviewer

@jander-msft jander-msft jander-msft approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Metric based triggers interfere with /metrics

3 participants

@wiktork @jander-msft @kkeirstead