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

Add docs for EF Core metrics #41358

Closed
wants to merge 4 commits into from
Closed

Add docs for EF Core metrics #41358

wants to merge 4 commits into from

Conversation

cincuranet
Copy link
Contributor

@cincuranet cincuranet commented Jun 11, 2024

@cincuranet cincuranet requested review from tommcdon and a team as code owners June 11, 2024 07:05
@dotnet-bot dotnet-bot added this to the June 2024 milestone Jun 11, 2024
@cincuranet cincuranet requested review from noahfalk and a team June 11, 2024 07:05
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

Looks good to me, one question inline.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

@cincuranet shouldn't we document these inside the EF docs, and point to that page from the .NET docs (i.e. from docs/core/diagnostics/built-in-metrics.md)?

Note that we already have an EF docs page on event counters; I'd merge this information into there, renaming it to "metrics" and keeping the current event counter information as a legacy thing at the bottom. Otherwise, as things are, people looking at the EF docs have no idea about the new metrics, and only see the event counter stuff, with no mention of it being legacy.

If we go down this path of an EF metrics page, I'd probably start with a quick intro mentioning the new support from EF 9.0, and the legacy event counters being available in the older versions (we can also point to this page for a comparison of the two APIs).

@cincuranet
Copy link
Contributor Author

shouldn't we document these inside the EF docs, and point to that page from the .NET docs (i.e. from docs/core/diagnostics/built-in-metrics.md)?

@noahfalk requested it here and ASP.NET Core has it here as well. 🤷‍♂️

Note that we already have an EF docs page on event counters; I'd merge this information into there, renaming it to "metrics" and keeping the current event counter information as a legacy thing at the bottom. Otherwise, as things are, people looking at the EF docs have no idea about the new metrics, and only see the event counter stuff, with no mention of it being legacy.

We can as well link to here from EF Core docs.

@roji
Copy link
Member

roji commented Jun 11, 2024

@noahfalk requested it here and ASP.NET Core has it here as well. 🤷‍♂️
[...]
We can as well link to here from EF Core docs.

So far we've generally been keeping EF docs, well, in the EF docs - after all this stuff is part of EF.. I don't think I'd want an EF user to need to go out of the EF docs sites just because they want to see the metrics we support...

@noahfalk would you be against the docs for the EF metrics living inside a page in the EF docs, and being linked to from this page?

@noahfalk
Copy link
Member

@noahfalk would you be against the docs for the EF metrics living inside a page in the EF docs, and being linked to from this page?

Thats just fine by me. I don't think readers will care what the path to the content is as long as the link is there.

@cincuranet
Copy link
Contributor Author

OK. Will redo it.

@cincuranet cincuranet closed this Jun 11, 2024
@roji
Copy link
Member

roji commented Jun 11, 2024

Thanks @cincuranet


| Name | Instrument Type | Unit (UCUM) | Description |
| -------- | --------------- | ----------- | -------------- |
| `microsoft.entityframeworkcore.compiled_query_cache_hit_rate` | ObservableGauge | `%` | Hit rate - since last observation - for the compiled query cache. |
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing "since last observation" means since the last time the ObservableGauge callback function was called? Not a doc issue, but that may be a functional/design issue for this metric. There is no guarantee that only one observer exists or what the relative timings are between observations. For example imagine the app developer sets up OpenTelemetry polling at 1 minute intervals, but then sometimes they also run dotnet-counters reporting at 1 second intervals. Your callback might get invoked:

t=0:01 (dotnet-counters)
t=0:02 (dotnet-counters)
...
t=0:59 (dotnet-counters)
t=1:00 (dotnet-counters)
t=1:00 (OpenTelemetry)
t=1:01 (dotnet-counters)

Those two calls at the 1 minute mark probably aren't going to be exactly the same time, maybe 10 microseconds happens to pass between them. OpenTelemetry is now reporting the cache hit rate for a period of time 10 microseconds long which might well be 0 hits of 0 attempts.

You generally want to pick metrics that do not vary based on when the metric was last queried because that is an unpredictable time interval. You could report total hits and total attempts since the cache initialized and let users do a query to compute (hits_now-hits_last)/(total_now-total_last). Alternatively you could do the % hit rate over a time window that is not based on past observations. Hit rate since process start, hit rate in the last minute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I guess we have the same problem with "old" event counters as well. I added dotnet/efcore#33959 to keep track of it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I wouldn't worry about fixing the old event counters at this point, better just leave them as they are - but we should fix the new metric before releasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants