Skip to content

Revert correct Meter name#5403

Merged
evgenyfedorov2 merged 3 commits into
dotnet:mainfrom
gitmln:main
Sep 6, 2024
Merged

Revert correct Meter name#5403
evgenyfedorov2 merged 3 commits into
dotnet:mainfrom
gitmln:main

Conversation

@gitmln
Copy link
Copy Markdown
Contributor

@gitmln gitmln commented Sep 5, 2024 •
edited by RussKie
Loading

Fixes #5404

Corrects a bug with wrong meter name that has been changed from Microsoft.Extensions.Diagnostics.ResourceMonitoring to ResourceMonitoring

Microsoft Reviewers: Open in CodeFlow

@gitmln gitmln mentioned this pull request Sep 5, 2024
Closed
@gitmln
Copy link
Copy Markdown
Contributor Author

gitmln commented Sep 5, 2024

Fixes #5404

@gitmln
Copy link
Copy Markdown
Contributor Author

gitmln commented Sep 5, 2024

@dotnet-policy-service agree

@evgenyfedorov2
Copy link
Copy Markdown
Member

evgenyfedorov2 commented Sep 6, 2024

Hi @gitmln,

Thank you for your contribution! Could you please add unit tests for the change made? We would like to avoid repeating this bug in the future.

evgenyfedorov2
evgenyfedorov2 approved these changes Sep 6, 2024
View reviewed changes
Comment thread ...soft.Extensions.Diagnostics.ResourceMonitoring.Tests/Windows/WindowsSnapshotProviderTests.cs Outdated
Assert.InRange(usage, 0, long.MaxValue);
}

[Fact]
Copy link
Copy Markdown
Member

@evgenyfedorov2 evgenyfedorov2 Sep 6, 2024

Choose a reason for hiding this comment

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

Should be [ConditionalFact] to support run on a certain platform only

Copy link
Copy Markdown
Contributor Author

@gitmln gitmln Sep 6, 2024

Choose a reason for hiding this comment

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

It turns out that all tests for WindowsContainerSnapshotProvider support running on Linux/MacOS. In fact, as I checked a pipeline, they are being executed on Ubuntu without a problem:
image

Question is should they? But I think this out of scope of this PR.

Copy link
Copy Markdown
Contributor

@RussKie RussKie Sep 6, 2024

Choose a reason for hiding this comment

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

Good observation.
No, that's no correct. The Windows-specific tests should have the following decoration:

[OSSkipCondition(OperatingSystems.Linux | OperatingSystems.MacOSX, SkipReason = "Windows specific tests")]

@evgenyfedorov2 evgenyfedorov2 merged commit 858624b into dotnet:main Sep 6, 2024
@dotnet dotnet deleted a comment from github-actions Bot Sep 6, 2024
@dotnet dotnet deleted a comment from github-actions Bot Sep 6, 2024
@dotnet dotnet deleted a comment from github-actions Bot Sep 6, 2024
@joperezr
Copy link
Copy Markdown
Member

joperezr commented Sep 6, 2024

/backport to release/8.9

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 6, 2024

Started backporting to release/8.9: https://github.com/dotnet/extensions/actions/runs/10741673708

@github-actions github-actions Bot mentioned this pull request Sep 6, 2024
Merged
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@evgenyfedorov2 evgenyfedorov2 evgenyfedorov2 approved these changes

+1 more reviewer

@RussKie RussKie RussKie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@gitmln gitmln

Labels

area-telemetry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Microsoft.Extensions.Diagnostics.ResourceMonitoring meter name changed to ResourceMonitoring

4 participants

@gitmln @evgenyfedorov2 @joperezr @RussKie