Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Aug 1, 2025

Changes:

@Copilot Copilot AI review requested due to automatic review settings August 1, 2025 09:49
@JamesNK JamesNK added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Aug 1, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces comprehensive improvements to the server memory pool system with enhanced metrics capabilities and configuration options. The key motivation is to provide better observability and tracking of memory pool usage across different components.

  • Adds MemoryPoolOptions class to enable specifying an owner for memory pools, improving metrics tagging and diagnostics
  • Replaces the old PinnedBlockMemoryPoolMetrics with a new shared MemoryPoolMetrics instance registered in DI, centralizing metric collection
  • Updates all metric names based on OpenTelemetry semantic conventions feedback and changes counter types from uint to ulong to prevent overflow

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
PinnedBlockMemoryPoolMetrics.cs Removed old metrics class that was specific to pinned block pools
MemoryPoolMetrics.cs New shared metrics class with owner tagging support and updated metric names
PinnedBlockMemoryPool.cs Updated to use new metrics system and changed counter types to ulong
IMemoryPoolFactory.cs / MemoryPoolOptions.cs Added new interface signature and options class for pool configuration
Various transport files Updated to pass MemoryPoolOptions with appropriate owner identifiers
DI registration files Added MemoryPoolMetrics to dependency injection containers
Test files Updated tests to work with new metrics system and verify owner tagging

/// <param name="options">Options for configuring the memory pool.</param>
/// <returns>A new memory pool instance.</returns>
MemoryPool<T> Create();
MemoryPool<T> Create(MemoryPoolOptions options);
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking this should be optional so you don't have to new up options every time you create a pool? Are your thoughts that since you shouldn't be creating a new pool very often it's fine to require the small allocation cost?

Copy link
Member Author

@JamesNK JamesNK Aug 2, 2025

Choose a reason for hiding this comment

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

Sure, it could be optional. That would follow what Channel has:

image

Do you think there should be two create methods on the interface, or one with the options as optional?

_usedMemoryCounter = _meter.CreateUpDownCounter<long>(
UsedMemoryName,
unit: "By",
description: "Number of bytes that are currently used by the pool.");
Copy link
Member

Choose a reason for hiding this comment

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

I think "currently stored by the pool." would be less ambiguous.

Copy link
Member Author

@JamesNK JamesNK Aug 2, 2025

Choose a reason for hiding this comment

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

What do you think of aspnetcore.memory_pool.used vs aspnetcore.memory_pool.pooled?

The used name came from another metric: jvm.memory.used. But if I think about it more, the meaning is different here. jvm.memory.used reports how much memory is used vs this metric which is how much memory is sitting idle in the pool.

For example, name and description could be:

        _usedMemoryCounter = _meter.CreateUpDownCounter<long>(
            "aspnetcore.memory_pool.pooled",
            unit: "By",
            description: "Number of bytes currently held by the pool and available for reuse.");

_endpoint = endpoint;
_options = options;
_memoryPool = options.MemoryPoolFactory.Create();
_memoryPool = options.MemoryPoolFactory.Create(new MemoryPoolOptions { Owner = "kestrel" });
Copy link
Member

Choose a reason for hiding this comment

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

Should these be more specific; kestrel_namedpipe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Originally I had them be specific, e.g. "namedpipe", but the memory pool is set on the connection that is created, which is then used throughout Kestrel for other reasons. I think it would be confusing if a memory pool that says it is owned by named pipes transport is then used by HTTPs middleware for example.

@BrennanConroy
Copy link
Member

Changes still look good 👍

@JamesNK
Copy link
Member Author

JamesNK commented Aug 8, 2025

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@JamesNK JamesNK merged commit ef2f62f into main Aug 9, 2025
29 checks passed
@JamesNK JamesNK deleted the jamesnk/memorypool-metrics branch August 9, 2025 01:00
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-rc1 milestone Aug 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants