Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/8.0-staging] Fix FormatQuantiles formatting in MetricsEventSource #99045

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 28, 2024

Backport of #98699 to release/8.0-staging

/cc @stephentoub

Customer Impact

  • Customer reported
  • Found internally

dotnet counters displays corrupted values when the application being monitored is running under certain cultures, like Finnish (Finland).

Regression

  • Yes
  • No

Regression introduced in .NET 8 as part of changes to add CompositeFormat. A line was erroneously changed to remove using the invariant culture when formatting a string and instead use the current culture.

Testing

New tests were added to validate the metrics event source is appropriately outputting the expected data under various cultures.

Risk

Low. The code was effectively restored to what it was previously using invariant culture instead of current culture.

These doubles need to be formatted with the invariant culture to meet consumer expectations around parsing them.
@ghost
Copy link

ghost commented Feb 28, 2024

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #98699 to release/8.0-staging

/cc @stephentoub

Customer Impact

  • Customer reported
  • Found internally

[Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.]

Regression

  • Yes
  • No

[If yes, specify when the regression was introduced. Provide the PR or commit if known.]

Testing

[How was the fix verified? How was the issue missed previously? What tests were added?]

Risk

[High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Diagnostics.Tracing

Milestone: -

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

@stephentoub please ensure adding the package authoring to this change.

package authoring

CC @ericstj @carlossanlop

@ericstj
Copy link
Member

ericstj commented Feb 28, 2024

I went ahead and added the servicing changes with the web editor.

@stephentoub stephentoub added the Servicing-consider Issue for next servicing release review label Mar 1, 2024
@stephentoub stephentoub added this to the 8.0.x milestone Mar 1, 2024
@stephentoub stephentoub added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 1, 2024
@carlossanlop
Copy link
Member

Friendly reminder that Monday March 11th is the Code Complete date for the April Release. This PR still needs the package authoring changes requested above.

@stephentoub
Copy link
Member

This PR still needs the package authoring changes requested above.

Is that different from what @ericstj added already?

@carlossanlop
Copy link
Member

My bad. It's ready to merge then.

@stephentoub
Copy link
Member

Build Analysis is red. I don't think those test failures are related to this, but can you confirm you've seen those same failures on other 8.0-staging PRs?

@carlossanlop
Copy link
Member

You read my mind. I was seeing that failure in other PRs and opened this issue to track it: #99378

I re-ran Build Analysis here and now most of them got grouped under that issue.

The other failures also had an issue open but the json was not getting detected correctly, so I edited it: #90639

@stephentoub
Copy link
Member

Ok, thanks.

@stephentoub stephentoub merged commit 4a1e5f4 into release/8.0-staging Mar 6, 2024
105 of 113 checks passed
@carlossanlop carlossanlop deleted the backport/pr-98699-to-release/8.0-staging branch March 6, 2024 20:03
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants