Dotnet8 metrics: proposals #2

lmolkova · 2023-07-21T23:33:19Z

No description provided.

docs/dotnet/dotnet-aspnet-metrics.md

docs/dotnet/dotnet-signalr-metrics.md

lmolkova · 2023-07-22T00:10:40Z

docs/dotnet/dotnet-signalr-metrics.md

-| `signalr.http_transport.transport` | string | TODO [2] | `TODO` | Recommended |
+| `signalr.signalr.transport` | string | SignalR transport - https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/docs/specs/TransportProtocols.md [1] | `ServerSentEvents` | Conditionally Required: if HTTP SignalR transport is used |
+| `dotnet.error.code` | string | General-purpose error code reported by .NET, as a starter it supports terminal states of .NET task https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskstatus. | `Canceled`; `RanToCompletion` | Recommended |
+| `http.response.status_code` | int | [HTTP response status code](https://tools.ietf.org/html/rfc7231#section-6). | `200` | Conditionally Required: If and only if one was received/sent. |


is there need for specific status code since this is http-specific metric?

I don't think there is but its my research queue.

I think we can remove this.

noahfalk

This feels like a mixed bag. Some stuff seemed like good improvements, others I am pretty skeptical about. Comments inline and more research in some areas still queued up.

docs/dotnet/dotnet-aspnet-metrics.md

noahfalk · 2023-07-25T11:44:51Z

docs/dotnet/dotnet-aspnet-metrics.md

 | `aspnet.handler` | string | TODO | `TODO` | Required |
+| `dotnet.error.code` | string | General-purpose error code reported by .NET, as a starter it supports terminal statuses of .NET task https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskstatus. | `Canceled`; `RanToCompletion` | Recommended |


The diagnostics_handler metrics were created for a specific purpose to help customers understand how exceptions in the pipeline were triaged by the exception handling middleware. I'm happy to rename exception.result to something else (perhaps aspnet.handler.result?), but the goal of enumeration values is to describe how the exception was dealt with. I would not change the semantics of the enumeration without an analysis in the context of the original feature ask and the overall function of the exception handling middleware.

The feature ask is here: dotnet/aspnetcore#48625
James' description of the solution is here: dotnet/aspnetcore#48648

The enumeration values as described by James:

Aborted - Error occurred because the request was aborted (OperationCanceledException + RequestAborted.IsCancellationRequested)

Skipped - Response has already started. Middleware can't do anything with the response so skips doing anything.

Handled - ExceptionHandlerMiddleware has some interesting rules here. An exception is considered handled if an IExceptionHandler returns true, IProblemDetailsService returns true, or the status code is anything other than 404. If an IExceptionHandler or IProblemDetailsService return true then the type name is set for the handler tag.
DeveloperExceptionMiddleware never reports exceptions it catches as handled

Unhandled - All of the above are false.

noahfalk · 2023-07-25T12:21:57Z

docs/dotnet/dotnet-aspnet-metrics.md

@@ -149,17 +146,12 @@ This metric is required.
 | `aspnet.rate_limiting.queued_requests` | UpDownCounter | `{request}` | Number of requests that are currently queued, waiting to acquire a rate limiting lease. [1] |

 **[1]:** Meter name is `Microsoft.AspNetCore.RateLimiting`
-
-**TODO: if they are queued, should they have reject reason ? **


No they shouldn't. In the original doc reject_reason isn't an attribute on this metric. I think it was added by mistake in your doc -> GH transfer.

It seems it's there

aspnet.rate_limiting.queued_requests.duration - Histogram - seconds aspnet.rate_limiting.policy aspnet.rate_limiting.reject_reason The duration of HTTP requests in a queue, waiting to acquire a rate limiting lease. Old name: rate-limiting-queued-request-duration

and in the code - https://github.com/dotnet/aspnetcore/blob/d7379f6998b9805772ed52a4792e87e6bf13ed9d/src/Middleware/RateLimiting/src/RateLimitingMetrics.cs#L146

You are referring to doc and code content for aspnet.rate_limiting.queued_request.duration counter, but this part of the markdown doc is talking about queued_requests, not queued_request.duration right?

docs/dotnet/dotnet-aspnet-metrics.md

noahfalk · 2023-07-25T13:15:57Z

docs/dotnet/dotnet-kestrel-metrics.md

+| [`network.protocol.version`](../general/attributes.md) | string | Version of the application layer protocol used. See note below. [1] | `3.1.1` | Recommended |
+| [`network.transport`](../general/attributes.md) | string | [OSI Transport Layer](https://osi-model.com/transport-layer/) or [Inter-process Communication method](https://en.wikipedia.org/wiki/Inter-process_communication). The value SHOULD be normalized to lowercase. | `tcp`; `udp` | Recommended |
+| [`network.type`](../general/attributes.md) | string | [OSI Network Layer](https://osi-model.com/network-layer/) or non-OSI equivalent. The value SHOULD be normalized to lowercase. | `ipv4`; `ipv6` | Recommended |
+| `tls.version` | string | TLS protocol version | `1.3` | Recommended |


This could also be an SSL version based on the type used in the code. The internal enumeration is here:
https://source.dot.net/#System.Net.Primitives/System/Net/SecureProtocols/SslEnumTypes.cs,bfabf9fcb928e856,references

Unless James confirmed that SSL was impossible based on some filtering elsewhere then I'd expect this attribute could have values like "ssl3.0" or "tls1.3". None of the SSL and TLS versions overlap right now, but its probably good not to be ambiguous about it.

noahfalk · 2023-07-25T13:22:15Z

docs/dotnet/dotnet-signalr-metrics.md


 <!-- tocstop -->

-## Metric: `signalr_http_transport.connection.duration`
+## Metric: `signalr.http.server.connection.duration`


I think http transport may have had a specific meaning but I need to do more research. Sorry James might have known this stuff offhand but I have to dig through the implementation and conversation history to get the context for all of these which makes each proposed change time consuming. Please still bring it up, thats just why I don't have quick answers for everything.

I did do more research here and I think this is the context:

SignalR is defined as a protocol that can operate over any reliable, in-order, duplex, message based transport

Right now the only implementations of transports all use http, but in three different variants (long polling, server sent events, and web sockets)

The Micrsoft.AspNetCore.Http.Connections assembly these metrics are implemented inside of has the logic to create those various http transport flavors. If there were some other non-http transport in the future I suspect it wouldn't be this assembly that implemented it.

It feels reasonable to omit the word transport and let http imply 'http-based transport'. I don't see any other likely interpretation that would create ambiguity.

docs/dotnet/dotnet-signalr-metrics.md

noahfalk · 2023-07-25T13:35:33Z

docs/dotnet/dotnet-signalr-metrics.md

-| `signalr.http_transport.transport` | string | TODO [2] | `TODO` | Recommended |
+| `signalr.signalr.transport` | string | SignalR transport - https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/docs/specs/TransportProtocols.md [1] | `ServerSentEvents` | Conditionally Required: if HTTP SignalR transport is used |
+| `dotnet.error.code` | string | General-purpose error code reported by .NET, as a starter it supports terminal states of .NET task https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskstatus. | `Canceled`; `RanToCompletion` | Recommended |
+| `http.response.status_code` | int | [HTTP response status code](https://tools.ietf.org/html/rfc7231#section-6). | `200` | Conditionally Required: If and only if one was received/sent. |


I don't think there is but its my research queue.

docs/dotnet/dotnet-signalr-metrics.md

Adding OpenAI metrics and fixing markdown errors

lmolkova added 8 commits July 21, 2023 13:41

First stub

daeaa1b

return http metrics back

1686a07

draft done

72ae36d

nits

d4a0053

nits

092b502

nits

42860c0

proposed changes

bac15e0

workaround build-tool errors

37dc1ec

lmolkova commented Jul 21, 2023

View reviewed changes

docs/dotnet/dotnet-aspnet-metrics.md Show resolved Hide resolved

lmolkova commented Jul 21, 2023

View reviewed changes

docs/dotnet/dotnet-signalr-metrics.md Outdated Show resolved Hide resolved

mroe fixes

9a9cbd2

lmolkova commented Jul 22, 2023

View reviewed changes

lmolkova added 3 commits July 21, 2023 18:17

update

7b96589

update

3515801

tls.version

13ce5a4

lmolkova changed the title ~~Dotnet8 metrics proposed diff~~ Dotnet8 metrics: proposals Jul 22, 2023

noahfalk reviewed Jul 25, 2023

View reviewed changes

noahfalk mentioned this pull request Jul 25, 2023

Dotnet8 metrics in otel format #1

Closed

lmolkova force-pushed the dotnet8-metrics branch 3 times, most recently from 0fd3372 to f85bfa8 Compare July 27, 2023 23:52

lmolkova closed this Aug 4, 2023

lmolkova pushed a commit that referenced this pull request Apr 10, 2024

Merge pull request #2 from drewby/ai

fdb3ba4

Adding OpenAI metrics and fixing markdown errors

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dotnet8 metrics: proposals #2

Dotnet8 metrics: proposals #2

lmolkova commented Jul 21, 2023

lmolkova Jul 22, 2023

noahfalk Jul 25, 2023

noahfalk Jul 28, 2023

noahfalk left a comment

noahfalk Jul 25, 2023

noahfalk Jul 25, 2023

lmolkova Jul 26, 2023

noahfalk Jul 26, 2023

noahfalk Jul 25, 2023

noahfalk Jul 25, 2023

noahfalk Jul 28, 2023 •

edited

Loading

noahfalk Jul 25, 2023

		\| `aspnet.handler` \| string \| TODO \| `TODO` \| Required \|
		\| `dotnet.error.code` \| string \| General-purpose error code reported by .NET, as a starter it supports terminal statuses of .NET task https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskstatus. \| `Canceled`; `RanToCompletion` \| Recommended \|

Dotnet8 metrics: proposals #2

Dotnet8 metrics: proposals #2

Conversation

lmolkova commented Jul 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahfalk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahfalk Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahfalk Jul 28, 2023 •

edited

Loading