[System.Diagnostics.DiagnosticSource] Implement metrics advice API - #102524

Merged
tarekgh merged 13 commits into
dotnet:mainfrom
CodeBlanch:metrics-histogramadvice
Jun 20, 2024
Merged

[System.Diagnostics.DiagnosticSource] Implement metrics advice API#102524
tarekgh merged 13 commits into
dotnet:mainfrom
CodeBlanch:metrics-histogramadvice

Conversation

@CodeBlanch

@CodeBlanchCodeBlanch commented May 21, 2024

Copy link
Copy Markdown
Contributor

@ghost

ghost commented May 21, 2024

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label May 21, 2024
tarekgh
tarekgh reviewed May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx Outdated
@CodeBlanch
CodeBlanch marked this pull request as ready for review May 21, 2024 21:16
tarekgh
tarekgh reviewed May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsAdviceTests.cs Outdated
tarekgh
tarekgh reviewed May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs Outdated
@tarekghtarekgh self-assigned this May 21, 2024
@tarekghtarekgh added this to the 9.0.0 milestone May 21, 2024
@tarekgh

tarekgh commented May 21, 2024

Copy link
Copy Markdown
Member

CC @noahfalk

tarekgh
tarekgh approved these changes May 21, 2024
View reviewed changes

@tarekghtarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks @CodeBlanch for providing the implementation.

@build-analysisbuild-analysisBot mentioned this pull request May 22, 2024
Closed
@CodeBlanch
CodeBlanch marked this pull request as draft June 4, 2024 17:53
This was referenced Jun 4, 2024
Closed
Open
Closed
@CodeBlanch
CodeBlanch marked this pull request as ready for review June 11, 2024 17:44
@tarekgh

tarekgh commented Jun 11, 2024

Copy link
Copy Markdown
Member

CC @noahfalk if need to take one final look before we merge.

noahfalk
noahfalk requested changes Jun 12, 2024
View reviewed changes

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've still got concerns about the 'init' keyword :)

Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
public IReadOnlyList<T>? HistogramBucketBoundaries
{
get => _HistogramBucketBoundaries;
init

@noahfalknoahfalkJun 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've still got concerns about 'init' due to the difficulty invoking it from .NET Framework while using the officially supported 7.3 C# language version. Maybe in the future we'll decide to drop support for .NET Framework but we've not agreed to that so far.

I maintain the suggestion to define this property with as 'set', not 'init' and if we want to protect against mutations then we can make a read-only copy.

@tarekghtarekghJun 12, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I humbly disagree. As I mentioned before, doing the allocation and copying is not good IMO especially we need to maintain that in the future too. We discussed that in the design review and didn't get any objection. Also, init is already used in the same library too.

@CodeBlanchCodeBlanchJun 12, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The code as-is already takes a copy of the data 😄 The issue I think set creates is more that it can't be changed after the first publish of a metric. What OTel is going to do is on instrument published take the advice and construct the buckets after which point they are fixed. So having the set could just be misleading for users thinking they can set it whenever. init really conveys the meaning nicely for what this class is doing.

I guess we could have set throw based on some internal state tracking if the thing was published? But then what if you hand the same advice class to multiple instruments? 🤔

@noahfalknoahfalkJun 19, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.

We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:

  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.

This was referenced Jun 12, 2024
Closed
Closed
noahfalk
noahfalk approved these changes Jun 19, 2024
View reviewed changes

@noahfalknoahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The objection to init is retracted, sorry for the delay while that discussion played out. Thanks @CodeBlanch!

Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
public IReadOnlyList<T>? HistogramBucketBoundaries
{
get => _HistogramBucketBoundaries;
init

@noahfalknoahfalkJun 19, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.

We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:

  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.

@tarekghtarekgh mentioned this pull request Jun 20, 2024
Closed
@tarekgh

tarekgh commented Jun 20, 2024

Copy link
Copy Markdown
Member

/ba-g logged #103784 for unrelated issue. Other failures are time out.

@tarekgh
tarekgh merged commit 7377eda into dotnet:mainJun 20, 2024
@build-analysisbuild-analysisBot mentioned this pull request Jun 20, 2024
Closed
@JamesNK

JamesNK commented Jun 20, 2024

Copy link
Copy Markdown
Member

Nice. I'll try using these next week in asp.net core. I'll provide feedback if I run into any problems.

@JamesNK

JamesNK commented Jun 20, 2024

Copy link
Copy Markdown
Member

@reyang Does opentelemetry-dotnet have an issue for consuming them?

@vishweshbankwar

vishweshbankwar commented Jun 20, 2024

Copy link
Copy Markdown
Contributor

@reyang Does opentelemetry-dotnet have an issue for consuming them?

@JamesNK - FYI open-telemetry/opentelemetry-dotnet#5487

rzikm pushed a commit to rzikm/dotnet-runtime that referenced this pull request Jun 24, 2024
@CodeBlanch@rzikm
[System.Diagnostics.DiagnosticSource] Implement metrics advice API (d…
396e87e
…otnet#102524)
* Prototype HistogramAdvice API.
* Updates.
* Add tests.
* Code review.
* Code review.
* Tweaks.
* Code review.
* Revisions.
* Tweaks.
* Code review.
* Add InstrumentAdvice ctor to ref.
@CodeBlanch
CodeBlanch deleted the metrics-histogramadvice branch June 25, 2024 20:18
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 26, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@tarekghtarekghtarekgh approved these changes
@noahfalknoahfalknoahfalk approved these changes
+1 more reviewer
@reyangreyangreyang left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

area-System.Diagnostics.Metriccommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

Add "hints" in Metric API to provide things like histogram bounds

7 participants

@CodeBlanch@tarekgh@JamesNK@vishweshbankwar@stephentoub@noahfalk@reyang
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
[System.Diagnostics.DiagnosticSource] Implement metrics advice API - #102524
Merged
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Jun 20, 2024
Merged
[System.Diagnostics.DiagnosticSource] Implement metrics advice API#102524
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Conversation
@CodeBlanch
@CodeBlanchCodeBlanch
commented
May 21, 2024

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
May 21, 2024

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 21, 2024
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx
Outdated

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
May 21, 2024 21:16
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsAdviceTests.cs
Outdated

tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs
Outdated

@tarekghtarekgh
self-assigned this
May 21, 2024
@tarekghtarekgh
added this to the 9.0.0 milestone
May 21, 2024
@tarekgh
tarekgh
commented
May 21, 2024

Copy link


Copy Markdown

Member
CC @noahfalk
tarekgh
tarekgh
approved these changes
May 21, 2024
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. Thanks @CodeBlanch for providing the implementation.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@CodeBlanch
CodeBlanch
marked this pull request as draft
June 4, 2024 17:53

This was referenced Jun 4, 2024

Closed


Open


Closed

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
June 11, 2024 17:44
@tarekgh
tarekgh
commented
Jun 11, 2024

Copy link


Copy Markdown

Member
CC @noahfalk if need to take one final look before we merge.
noahfalk
noahfalk
requested changes
Jun 12, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about the 'init' keyword :)
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 11, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about 'init' due to the difficulty invoking it from .NET Framework while using the officially supported 7.3 C# language version. Maybe in the future we'll decide to drop support for .NET Framework but we've not agreed to that so far.
I maintain the suggestion to define this property with as 'set', not 'init' and if we want to protect against mutations then we can make a read-only copy.
@tarekghtarekghJun 12, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I humbly disagree. As I mentioned before, doing the allocation and copying is not good IMO especially we need to maintain that in the future too. We discussed that in the design review and didn't get any objection. Also, init is already used in the same library too.
@CodeBlanchCodeBlanchJun 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The code as-is already takes a copy of the data 😄 The issue I think set creates is more that it can't be changed after the first publish of a metric. What OTel is going to do is on instrument published take the advice and construct the buckets after which point they are fixed. So having the set could just be misleading for users thinking they can set it whenever. init really conveys the meaning nicely for what this class is doing.
I guess we could have set throw based on some internal state tracking if the thing was published? But then what if you hand the same advice class to multiple instruments? 🤔
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.

This was referenced Jun 12, 2024

Closed


Closed

noahfalk
noahfalk
approved these changes
Jun 19, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The objection to init is retracted, sorry for the delay while that discussion played out. Thanks @CodeBlanch!
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.
@tarekghtarekgh
mentioned this pull request
Jun 20, 2024

Closed

@tarekgh
tarekgh
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
/ba-g logged #103784 for unrelated issue. Other failures are time out.
@tarekgh
tarekgh
merged commit 7377eda
into
dotnet:mainJun 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 20, 2024

Closed

@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Nice. I'll try using these next week in asp.net core. I'll provide feedback if I run into any problems.
@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@vishweshbankwar
vishweshbankwar
commented
Jun 20, 2024

Copy link


Copy Markdown

Contributor
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@JamesNK - FYI open-telemetry/opentelemetry-dotnet#5487
rzikm pushed a commit
to rzikm/dotnet-runtime
that referenced
this pull request
Jun 24, 2024
@CodeBlanch@rzikm
[System.Diagnostics.DiagnosticSource] Implement metrics advice API (d…
396e87e
…otnet#102524)
* Prototype HistogramAdvice API.
* Updates.
* Add tests.
* Code review.
* Code review.
* Tweaks.
* Code review.
* Revisions.
* Tweaks.
* Code review.
* Add InstrumentAdvice ctor to ref.
@CodeBlanch
CodeBlanch
deleted the

metrics-histogramadvice

branch
June 25, 2024 20:18
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 26, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tarekghtarekghtarekgh approved these changes
@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@reyangreyangreyang left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

area-System.Diagnostics.Metriccommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

Add "hints" in Metric API to provide things like histogram bounds


7 participants

@CodeBlanch@tarekgh@JamesNK@vishweshbankwar@stephentoub@noahfalk@reyang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[System.Diagnostics.DiagnosticSource] Implement metrics advice API - #102524
Merged
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Jun 20, 2024
Merged
[System.Diagnostics.DiagnosticSource] Implement metrics advice API#102524
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Conversation
@CodeBlanch
@CodeBlanchCodeBlanch
commented
May 21, 2024

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
May 21, 2024

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 21, 2024
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx
Outdated

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
May 21, 2024 21:16
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsAdviceTests.cs
Outdated

tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs
Outdated

@tarekghtarekgh
self-assigned this
May 21, 2024
@tarekghtarekgh
added this to the 9.0.0 milestone
May 21, 2024
@tarekgh
tarekgh
commented
May 21, 2024

Copy link


Copy Markdown

Member
CC @noahfalk
tarekgh
tarekgh
approved these changes
May 21, 2024
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. Thanks @CodeBlanch for providing the implementation.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@CodeBlanch
CodeBlanch
marked this pull request as draft
June 4, 2024 17:53

This was referenced Jun 4, 2024

Closed


Open


Closed

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
June 11, 2024 17:44
@tarekgh
tarekgh
commented
Jun 11, 2024

Copy link


Copy Markdown

Member
CC @noahfalk if need to take one final look before we merge.
noahfalk
noahfalk
requested changes
Jun 12, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about the 'init' keyword :)
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 11, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about 'init' due to the difficulty invoking it from .NET Framework while using the officially supported 7.3 C# language version. Maybe in the future we'll decide to drop support for .NET Framework but we've not agreed to that so far.
I maintain the suggestion to define this property with as 'set', not 'init' and if we want to protect against mutations then we can make a read-only copy.
@tarekghtarekghJun 12, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I humbly disagree. As I mentioned before, doing the allocation and copying is not good IMO especially we need to maintain that in the future too. We discussed that in the design review and didn't get any objection. Also, init is already used in the same library too.
@CodeBlanchCodeBlanchJun 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The code as-is already takes a copy of the data 😄 The issue I think set creates is more that it can't be changed after the first publish of a metric. What OTel is going to do is on instrument published take the advice and construct the buckets after which point they are fixed. So having the set could just be misleading for users thinking they can set it whenever. init really conveys the meaning nicely for what this class is doing.
I guess we could have set throw based on some internal state tracking if the thing was published? But then what if you hand the same advice class to multiple instruments? 🤔
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.

This was referenced Jun 12, 2024

Closed


Closed

noahfalk
noahfalk
approved these changes
Jun 19, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The objection to init is retracted, sorry for the delay while that discussion played out. Thanks @CodeBlanch!
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.
@tarekghtarekgh
mentioned this pull request
Jun 20, 2024

Closed

@tarekgh
tarekgh
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
/ba-g logged #103784 for unrelated issue. Other failures are time out.
@tarekgh
tarekgh
merged commit 7377eda
into
dotnet:mainJun 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 20, 2024

Closed

@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Nice. I'll try using these next week in asp.net core. I'll provide feedback if I run into any problems.
@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@vishweshbankwar
vishweshbankwar
commented
Jun 20, 2024

Copy link


Copy Markdown

Contributor
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@JamesNK - FYI open-telemetry/opentelemetry-dotnet#5487
rzikm pushed a commit
to rzikm/dotnet-runtime
that referenced
this pull request
Jun 24, 2024
@CodeBlanch@rzikm
[System.Diagnostics.DiagnosticSource] Implement metrics advice API (d…
396e87e
…otnet#102524)
* Prototype HistogramAdvice API.
* Updates.
* Add tests.
* Code review.
* Code review.
* Tweaks.
* Code review.
* Revisions.
* Tweaks.
* Code review.
* Add InstrumentAdvice ctor to ref.
@CodeBlanch
CodeBlanch
deleted the

metrics-histogramadvice

branch
June 25, 2024 20:18
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 26, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tarekghtarekghtarekgh approved these changes
@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@reyangreyangreyang left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

area-System.Diagnostics.Metriccommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

Add "hints" in Metric API to provide things like histogram bounds


7 participants

@CodeBlanch@tarekgh@JamesNK@vishweshbankwar@stephentoub@noahfalk@reyang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[System.Diagnostics.DiagnosticSource] Implement metrics advice API - #102524
Merged
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Jun 20, 2024
Merged
[System.Diagnostics.DiagnosticSource] Implement metrics advice API#102524
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Conversation
@CodeBlanch
@CodeBlanchCodeBlanch
commented
May 21, 2024

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
May 21, 2024

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 21, 2024
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx
Outdated

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
May 21, 2024 21:16
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsAdviceTests.cs
Outdated

tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs
Outdated

@tarekghtarekgh
self-assigned this
May 21, 2024
@tarekghtarekgh
added this to the 9.0.0 milestone
May 21, 2024
@tarekgh
tarekgh
commented
May 21, 2024

Copy link


Copy Markdown

Member
CC @noahfalk
tarekgh
tarekgh
approved these changes
May 21, 2024
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. Thanks @CodeBlanch for providing the implementation.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@CodeBlanch
CodeBlanch
marked this pull request as draft
June 4, 2024 17:53

This was referenced Jun 4, 2024

Closed


Open


Closed

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
June 11, 2024 17:44
@tarekgh
tarekgh
commented
Jun 11, 2024

Copy link


Copy Markdown

Member
CC @noahfalk if need to take one final look before we merge.
noahfalk
noahfalk
requested changes
Jun 12, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about the 'init' keyword :)
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 11, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about 'init' due to the difficulty invoking it from .NET Framework while using the officially supported 7.3 C# language version. Maybe in the future we'll decide to drop support for .NET Framework but we've not agreed to that so far.
I maintain the suggestion to define this property with as 'set', not 'init' and if we want to protect against mutations then we can make a read-only copy.
@tarekghtarekghJun 12, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I humbly disagree. As I mentioned before, doing the allocation and copying is not good IMO especially we need to maintain that in the future too. We discussed that in the design review and didn't get any objection. Also, init is already used in the same library too.
@CodeBlanchCodeBlanchJun 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The code as-is already takes a copy of the data 😄 The issue I think set creates is more that it can't be changed after the first publish of a metric. What OTel is going to do is on instrument published take the advice and construct the buckets after which point they are fixed. So having the set could just be misleading for users thinking they can set it whenever. init really conveys the meaning nicely for what this class is doing.
I guess we could have set throw based on some internal state tracking if the thing was published? But then what if you hand the same advice class to multiple instruments? 🤔
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.

This was referenced Jun 12, 2024

Closed


Closed

noahfalk
noahfalk
approved these changes
Jun 19, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The objection to init is retracted, sorry for the delay while that discussion played out. Thanks @CodeBlanch!
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.
@tarekghtarekgh
mentioned this pull request
Jun 20, 2024

Closed

@tarekgh
tarekgh
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
/ba-g logged #103784 for unrelated issue. Other failures are time out.
@tarekgh
tarekgh
merged commit 7377eda
into
dotnet:mainJun 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 20, 2024

Closed

@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Nice. I'll try using these next week in asp.net core. I'll provide feedback if I run into any problems.
@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@vishweshbankwar
vishweshbankwar
commented
Jun 20, 2024

Copy link


Copy Markdown

Contributor
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@JamesNK - FYI open-telemetry/opentelemetry-dotnet#5487
rzikm pushed a commit
to rzikm/dotnet-runtime
that referenced
this pull request
Jun 24, 2024
@CodeBlanch@rzikm
[System.Diagnostics.DiagnosticSource] Implement metrics advice API (d…
396e87e
…otnet#102524)
* Prototype HistogramAdvice API.
* Updates.
* Add tests.
* Code review.
* Code review.
* Tweaks.
* Code review.
* Revisions.
* Tweaks.
* Code review.
* Add InstrumentAdvice ctor to ref.
@CodeBlanch
CodeBlanch
deleted the

metrics-histogramadvice

branch
June 25, 2024 20:18
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 26, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tarekghtarekghtarekgh approved these changes
@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@reyangreyangreyang left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

area-System.Diagnostics.Metriccommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

Add "hints" in Metric API to provide things like histogram bounds


7 participants

@CodeBlanch@tarekgh@JamesNK@vishweshbankwar@stephentoub@noahfalk@reyang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
[System.Diagnostics.DiagnosticSource] Implement metrics advice API - #102524
Merged
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Jun 20, 2024
Merged
[System.Diagnostics.DiagnosticSource] Implement metrics advice API#102524
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Conversation
@CodeBlanch
@CodeBlanchCodeBlanch
commented
May 21, 2024

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
May 21, 2024

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 21, 2024
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx
Outdated

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
May 21, 2024 21:16
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsAdviceTests.cs
Outdated

tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs
Outdated

@tarekghtarekgh
self-assigned this
May 21, 2024
@tarekghtarekgh
added this to the 9.0.0 milestone
May 21, 2024
@tarekgh
tarekgh
commented
May 21, 2024

Copy link


Copy Markdown

Member
CC @noahfalk
tarekgh
tarekgh
approved these changes
May 21, 2024
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. Thanks @CodeBlanch for providing the implementation.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@CodeBlanch
CodeBlanch
marked this pull request as draft
June 4, 2024 17:53

This was referenced Jun 4, 2024

Closed


Open


Closed

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
June 11, 2024 17:44
@tarekgh
tarekgh
commented
Jun 11, 2024

Copy link


Copy Markdown

Member
CC @noahfalk if need to take one final look before we merge.
noahfalk
noahfalk
requested changes
Jun 12, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about the 'init' keyword :)
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 11, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about 'init' due to the difficulty invoking it from .NET Framework while using the officially supported 7.3 C# language version. Maybe in the future we'll decide to drop support for .NET Framework but we've not agreed to that so far.
I maintain the suggestion to define this property with as 'set', not 'init' and if we want to protect against mutations then we can make a read-only copy.
@tarekghtarekghJun 12, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I humbly disagree. As I mentioned before, doing the allocation and copying is not good IMO especially we need to maintain that in the future too. We discussed that in the design review and didn't get any objection. Also, init is already used in the same library too.
@CodeBlanchCodeBlanchJun 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The code as-is already takes a copy of the data 😄 The issue I think set creates is more that it can't be changed after the first publish of a metric. What OTel is going to do is on instrument published take the advice and construct the buckets after which point they are fixed. So having the set could just be misleading for users thinking they can set it whenever. init really conveys the meaning nicely for what this class is doing.
I guess we could have set throw based on some internal state tracking if the thing was published? But then what if you hand the same advice class to multiple instruments? 🤔
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.

This was referenced Jun 12, 2024

Closed


Closed

noahfalk
noahfalk
approved these changes
Jun 19, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The objection to init is retracted, sorry for the delay while that discussion played out. Thanks @CodeBlanch!
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.
@tarekghtarekgh
mentioned this pull request
Jun 20, 2024

Closed

@tarekgh
tarekgh
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
/ba-g logged #103784 for unrelated issue. Other failures are time out.
@tarekgh
tarekgh
merged commit 7377eda
into
dotnet:mainJun 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 20, 2024

Closed

@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Nice. I'll try using these next week in asp.net core. I'll provide feedback if I run into any problems.
@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@vishweshbankwar
vishweshbankwar
commented
Jun 20, 2024

Copy link


Copy Markdown

Contributor
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@JamesNK - FYI open-telemetry/opentelemetry-dotnet#5487
rzikm pushed a commit
to rzikm/dotnet-runtime
that referenced
this pull request
Jun 24, 2024
@CodeBlanch@rzikm
[System.Diagnostics.DiagnosticSource] Implement metrics advice API (d…
396e87e
…otnet#102524)
* Prototype HistogramAdvice API.
* Updates.
* Add tests.
* Code review.
* Code review.
* Tweaks.
* Code review.
* Revisions.
* Tweaks.
* Code review.
* Add InstrumentAdvice ctor to ref.
@CodeBlanch
CodeBlanch
deleted the

metrics-histogramadvice

branch
June 25, 2024 20:18
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 26, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tarekghtarekghtarekgh approved these changes
@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@reyangreyangreyang left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

area-System.Diagnostics.Metriccommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

Add "hints" in Metric API to provide things like histogram bounds


7 participants

@CodeBlanch@tarekgh@JamesNK@vishweshbankwar@stephentoub@noahfalk@reyang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[System.Diagnostics.DiagnosticSource] Implement metrics advice API - #102524
Merged
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Jun 20, 2024
Merged
[System.Diagnostics.DiagnosticSource] Implement metrics advice API#102524
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Conversation
@CodeBlanch
@CodeBlanchCodeBlanch
commented
May 21, 2024

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
May 21, 2024

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 21, 2024
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx
Outdated

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
May 21, 2024 21:16
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsAdviceTests.cs
Outdated

tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs
Outdated

@tarekghtarekgh
self-assigned this
May 21, 2024
@tarekghtarekgh
added this to the 9.0.0 milestone
May 21, 2024
@tarekgh
tarekgh
commented
May 21, 2024

Copy link


Copy Markdown

Member
CC @noahfalk
tarekgh
tarekgh
approved these changes
May 21, 2024
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. Thanks @CodeBlanch for providing the implementation.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@CodeBlanch
CodeBlanch
marked this pull request as draft
June 4, 2024 17:53

This was referenced Jun 4, 2024

Closed


Open


Closed

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
June 11, 2024 17:44
@tarekgh
tarekgh
commented
Jun 11, 2024

Copy link


Copy Markdown

Member
CC @noahfalk if need to take one final look before we merge.
noahfalk
noahfalk
requested changes
Jun 12, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about the 'init' keyword :)
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 11, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about 'init' due to the difficulty invoking it from .NET Framework while using the officially supported 7.3 C# language version. Maybe in the future we'll decide to drop support for .NET Framework but we've not agreed to that so far.
I maintain the suggestion to define this property with as 'set', not 'init' and if we want to protect against mutations then we can make a read-only copy.
@tarekghtarekghJun 12, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I humbly disagree. As I mentioned before, doing the allocation and copying is not good IMO especially we need to maintain that in the future too. We discussed that in the design review and didn't get any objection. Also, init is already used in the same library too.
@CodeBlanchCodeBlanchJun 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The code as-is already takes a copy of the data 😄 The issue I think set creates is more that it can't be changed after the first publish of a metric. What OTel is going to do is on instrument published take the advice and construct the buckets after which point they are fixed. So having the set could just be misleading for users thinking they can set it whenever. init really conveys the meaning nicely for what this class is doing.
I guess we could have set throw based on some internal state tracking if the thing was published? But then what if you hand the same advice class to multiple instruments? 🤔
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.

This was referenced Jun 12, 2024

Closed


Closed

noahfalk
noahfalk
approved these changes
Jun 19, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The objection to init is retracted, sorry for the delay while that discussion played out. Thanks @CodeBlanch!
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.
@tarekghtarekgh
mentioned this pull request
Jun 20, 2024

Closed

@tarekgh
tarekgh
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
/ba-g logged #103784 for unrelated issue. Other failures are time out.
@tarekgh
tarekgh
merged commit 7377eda
into
dotnet:mainJun 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 20, 2024

Closed

@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Nice. I'll try using these next week in asp.net core. I'll provide feedback if I run into any problems.
@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@vishweshbankwar
vishweshbankwar
commented
Jun 20, 2024

Copy link


Copy Markdown

Contributor
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@JamesNK - FYI open-telemetry/opentelemetry-dotnet#5487
rzikm pushed a commit
to rzikm/dotnet-runtime
that referenced
this pull request
Jun 24, 2024
@CodeBlanch@rzikm
[System.Diagnostics.DiagnosticSource] Implement metrics advice API (d…
396e87e
…otnet#102524)
* Prototype HistogramAdvice API.
* Updates.
* Add tests.
* Code review.
* Code review.
* Tweaks.
* Code review.
* Revisions.
* Tweaks.
* Code review.
* Add InstrumentAdvice ctor to ref.
@CodeBlanch
CodeBlanch
deleted the

metrics-histogramadvice

branch
June 25, 2024 20:18
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 26, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tarekghtarekghtarekgh approved these changes
@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@reyangreyangreyang left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

area-System.Diagnostics.Metriccommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

Add "hints" in Metric API to provide things like histogram bounds


7 participants

@CodeBlanch@tarekgh@JamesNK@vishweshbankwar@stephentoub@noahfalk@reyang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
[System.Diagnostics.DiagnosticSource] Implement metrics advice API - #102524
Merged
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Jun 20, 2024
Merged
[System.Diagnostics.DiagnosticSource] Implement metrics advice API#102524
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Conversation
@CodeBlanch
@CodeBlanchCodeBlanch
commented
May 21, 2024

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
May 21, 2024

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 21, 2024
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx
Outdated

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
May 21, 2024 21:16
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsAdviceTests.cs
Outdated

tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs
Outdated

@tarekghtarekgh
self-assigned this
May 21, 2024
@tarekghtarekgh
added this to the 9.0.0 milestone
May 21, 2024
@tarekgh
tarekgh
commented
May 21, 2024

Copy link


Copy Markdown

Member
CC @noahfalk
tarekgh
tarekgh
approved these changes
May 21, 2024
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. Thanks @CodeBlanch for providing the implementation.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@CodeBlanch
CodeBlanch
marked this pull request as draft
June 4, 2024 17:53

This was referenced Jun 4, 2024

Closed


Open


Closed

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
June 11, 2024 17:44
@tarekgh
tarekgh
commented
Jun 11, 2024

Copy link


Copy Markdown

Member
CC @noahfalk if need to take one final look before we merge.
noahfalk
noahfalk
requested changes
Jun 12, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about the 'init' keyword :)
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 11, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about 'init' due to the difficulty invoking it from .NET Framework while using the officially supported 7.3 C# language version. Maybe in the future we'll decide to drop support for .NET Framework but we've not agreed to that so far.
I maintain the suggestion to define this property with as 'set', not 'init' and if we want to protect against mutations then we can make a read-only copy.
@tarekghtarekghJun 12, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I humbly disagree. As I mentioned before, doing the allocation and copying is not good IMO especially we need to maintain that in the future too. We discussed that in the design review and didn't get any objection. Also, init is already used in the same library too.
@CodeBlanchCodeBlanchJun 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The code as-is already takes a copy of the data 😄 The issue I think set creates is more that it can't be changed after the first publish of a metric. What OTel is going to do is on instrument published take the advice and construct the buckets after which point they are fixed. So having the set could just be misleading for users thinking they can set it whenever. init really conveys the meaning nicely for what this class is doing.
I guess we could have set throw based on some internal state tracking if the thing was published? But then what if you hand the same advice class to multiple instruments? 🤔
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.

This was referenced Jun 12, 2024

Closed


Closed

noahfalk
noahfalk
approved these changes
Jun 19, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The objection to init is retracted, sorry for the delay while that discussion played out. Thanks @CodeBlanch!
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.
@tarekghtarekgh
mentioned this pull request
Jun 20, 2024

Closed

@tarekgh
tarekgh
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
/ba-g logged #103784 for unrelated issue. Other failures are time out.
@tarekgh
tarekgh
merged commit 7377eda
into
dotnet:mainJun 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 20, 2024

Closed

@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Nice. I'll try using these next week in asp.net core. I'll provide feedback if I run into any problems.
@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@vishweshbankwar
vishweshbankwar
commented
Jun 20, 2024

Copy link


Copy Markdown

Contributor
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@JamesNK - FYI open-telemetry/opentelemetry-dotnet#5487
rzikm pushed a commit
to rzikm/dotnet-runtime
that referenced
this pull request
Jun 24, 2024
@CodeBlanch@rzikm
[System.Diagnostics.DiagnosticSource] Implement metrics advice API (d…
396e87e
…otnet#102524)
* Prototype HistogramAdvice API.
* Updates.
* Add tests.
* Code review.
* Code review.
* Tweaks.
* Code review.
* Revisions.
* Tweaks.
* Code review.
* Add InstrumentAdvice ctor to ref.
@CodeBlanch
CodeBlanch
deleted the

metrics-histogramadvice

branch
June 25, 2024 20:18
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 26, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tarekghtarekghtarekgh approved these changes
@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@reyangreyangreyang left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

area-System.Diagnostics.Metriccommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

Add "hints" in Metric API to provide things like histogram bounds


7 participants

@CodeBlanch@tarekgh@JamesNK@vishweshbankwar@stephentoub@noahfalk@reyang

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
[System.Diagnostics.DiagnosticSource] Implement metrics advice API - #102524
Merged
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Jun 20, 2024
Merged
[System.Diagnostics.DiagnosticSource] Implement metrics advice API#102524
tarekgh merged 13 commits into
dotnet:mainfrom 
CodeBlanch:metrics-histogramadvice
Conversation
@CodeBlanch
@CodeBlanchCodeBlanch
commented
May 21, 2024

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
May 21, 2024

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 21, 2024
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/src/Resources/Strings.resx
Outdated

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
May 21, 2024 21:16
tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsAdviceTests.cs
Outdated

tarekgh
tarekgh
reviewed
May 21, 2024
View reviewed changes
Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs
Outdated

@tarekghtarekgh
self-assigned this
May 21, 2024
@tarekghtarekgh
added this to the 9.0.0 milestone
May 21, 2024
@tarekgh
tarekgh
commented
May 21, 2024

Copy link


Copy Markdown

Member
CC @noahfalk
tarekgh
tarekgh
approved these changes
May 21, 2024
View reviewed changes
@tarekghtarekgh
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM. Thanks @CodeBlanch for providing the implementation.
@build-analysisbuild-analysisBot
mentioned this pull request
May 22, 2024

Closed

@CodeBlanch
CodeBlanch
marked this pull request as draft
June 4, 2024 17:53

This was referenced Jun 4, 2024

Closed


Open


Closed

@CodeBlanch
CodeBlanch
marked this pull request as ready for review
June 11, 2024 17:44
@tarekgh
tarekgh
commented
Jun 11, 2024

Copy link


Copy Markdown

Member
CC @noahfalk if need to take one final look before we merge.
noahfalk
noahfalk
requested changes
Jun 12, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about the 'init' keyword :)
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 11, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've still got concerns about 'init' due to the difficulty invoking it from .NET Framework while using the officially supported 7.3 C# language version. Maybe in the future we'll decide to drop support for .NET Framework but we've not agreed to that so far.
I maintain the suggestion to define this property with as 'set', not 'init' and if we want to protect against mutations then we can make a read-only copy.
@tarekghtarekghJun 12, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I humbly disagree. As I mentioned before, doing the allocation and copying is not good IMO especially we need to maintain that in the future too. We discussed that in the design review and didn't get any objection. Also, init is already used in the same library too.
@CodeBlanchCodeBlanchJun 12, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The code as-is already takes a copy of the data 😄 The issue I think set creates is more that it can't be changed after the first publish of a metric. What OTel is going to do is on instrument published take the advice and construct the buckets after which point they are fixed. So having the set could just be misleading for users thinking they can set it whenever. init really conveys the meaning nicely for what this class is doing.
I guess we could have set throw based on some internal state tracking if the thing was published? But then what if you hand the same advice class to multiple instruments? 🤔
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.

This was referenced Jun 12, 2024

Closed


Closed

noahfalk
noahfalk
approved these changes
Jun 19, 2024
View reviewed changes
@noahfalknoahfalk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

The objection to init is retracted, sorry for the delay while that discussion played out. Thanks @CodeBlanch!
Comment thread...aries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/InstrumentAdvice.cs
 public IReadOnlyList<T>? HistogramBucketBoundaries
 {
 get => _HistogramBucketBoundaries;
 init
@noahfalknoahfalkJun 19, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

We had a fair bit of internal discussion around init and concluded that yes, we are going to use the new language feature here.
We know that the presence of init makes it less straightforward to consume from projects using older runtime versions such as .NET Framework 4.8. By default those older versions target C# language version 7.3 which doesn't support the init keyword. Our overall goal is to design new feature APIs favoring projects that also use new languages and runtimes if a tradeoff has to be made. For developers who remain on older runtime versions (such as .NET Framework) there are a couple options:
  • The most conservative choice would be not to use new features at all if they require a new language version and stick with the features that were available at the time a given runtime was released.
  • An alternative that I think many developers would find preferable is to target their project at a newer C# language version when needed. Although old runtimes don't support every new language feature, the compiler does do build-time error checking to prevent accidental usage of new features on runtime versions that are known to be incompatible. The .NET team also validates our features in .NET Framework test apps that invoke the new APIs using the latest C# language version. In practice we see many .NET developers update their C# language version and are very happy with the results.
@tarekghtarekgh
mentioned this pull request
Jun 20, 2024

Closed

@tarekgh
tarekgh
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
/ba-g logged #103784 for unrelated issue. Other failures are time out.
@tarekgh
tarekgh
merged commit 7377eda
into
dotnet:mainJun 20, 2024
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 20, 2024

Closed

@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
Nice. I'll try using these next week in asp.net core. I'll provide feedback if I run into any problems.
@JamesNK
JamesNK
commented
Jun 20, 2024

Copy link


Copy Markdown

Member
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@vishweshbankwar
vishweshbankwar
commented
Jun 20, 2024

Copy link


Copy Markdown

Contributor
@reyang Does opentelemetry-dotnet have an issue for consuming them?
@JamesNK - FYI open-telemetry/opentelemetry-dotnet#5487
rzikm pushed a commit
to rzikm/dotnet-runtime
that referenced
this pull request
Jun 24, 2024
@CodeBlanch@rzikm
[System.Diagnostics.DiagnosticSource] Implement metrics advice API (d…
396e87e
…otnet#102524)
* Prototype HistogramAdvice API.
* Updates.
* Add tests.
* Code review.
* Code review.
* Tweaks.
* Code review.
* Revisions.
* Tweaks.
* Code review.
* Add InstrumentAdvice ctor to ref.
@CodeBlanch
CodeBlanch
deleted the

metrics-histogramadvice

branch
June 25, 2024 20:18
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 26, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@tarekghtarekghtarekgh approved these changes
@noahfalknoahfalknoahfalk approved these changes

+1 more reviewer

@reyangreyangreyang left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

area-System.Diagnostics.Metriccommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

Add "hints" in Metric API to provide things like histogram bounds


7 participants

@CodeBlanch@tarekgh@JamesNK@vishweshbankwar@stephentoub@noahfalk@reyang