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 .NET metrics #283

Merged
merged 25 commits into from
Dec 11, 2023
Merged

Add .NET metrics #283

merged 25 commits into from
Dec 11, 2023

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Aug 24, 2023

.NET 8 (currently in release-candidate status) introduced a bunch of client and server metrics mostly related to HTTP inside the platform. This PR documents them.

Open questions:

  • What's the right location for conventions like these which are not guidance, but documentation? We have a similar story for Kafka broker or AWS SDK
  • Assuming OTel semconv will get a breaking change (pre- or after- semconv stability), .NET may or may not adopt this change and do so at their own pace: we'll need a way to keep .NET part unchanged in otel semconv repo

Why keep .NET semconv here:

  • Discoverability: This is how .NET users can discover OTel and OTel users can discover there is out-of-the-box metrics in .NET
  • Unification: keeping semconv outside makes it harder to reuse common attributes and follow naming patterns
  • Tooling: yaml schema and tooling around it works well only if new semconv are defined along with common ones

@lmolkova
Copy link
Contributor Author

Discussed at Semconv SIG meeting:

What's the right location for conventions like these which are not guidance, but documentation? We have a similar story for Kafka broker or AWS SDK

  • OTel would need approvals from .NET metric owners (@noahfalk @JamesNK @antonfirsov)
  • OTel would also need .NET team to own this set of documents: review PRs, keep up-to-date, and discuss/plan changes, (@reyang to confirm)

Assuming OTel semconv will get a breaking change (pre- or after- semconv stability), .NET may or may not adopt this change and do so at their own pace: we'll need a way to keep .NET part unchanged in otel semconv repo

We'll need to find the balance, but it's not a blocker

@JamesNK
Copy link
Contributor

JamesNK commented Sep 9, 2023

Discussed at Semconv SIG meeting:

@lmolkova Thanks for moving this forward.

OTel would need approvals from .NET metric owners (@noahfalk @JamesNK @antonfirsov)

It looks good to me.

OTel would also need .NET team to own this set of documents: review PRs, keep up-to-date, and discuss/plan changes, (@reyang to confirm)

Also fine. We'll be responsible for keeping it up to date. @noahfalk to confirm.

I think the plan for .NET docs and metrics will be to have a list of meters and counter type+name+descriptions on learn.microsoft.com. It will act as an easy-to-read summary.

Then, we'll link to OpenTelemetry semantic conventions for details about each counter (unit, attributes, example values).

.NET 8 RC1 will ship with these counter names and values. It should be released in the next week. After that, this PR is good to merge, and we can update the learn.microsoft.com docs to point towards them. Are there any more questions to resolve?

@noahfalk
Copy link
Contributor

Also fine. We'll be responsible for keeping it up to date. @noahfalk to confirm.

Yeah, that seems fine. One caveat is that I'm not expecting to be actively monitoring for PRs that weren't originated by the .NET team so you may need to ping me if something needs review. I assume other people making PRs to the .NET area of the content wouldn't occur often.

@trask
Copy link
Member

trask commented Sep 11, 2023

One caveat is that I'm not expecting to be actively monitoring for PRs that weren't originated by the .NET team so you may need to ping me if something needs review. I assume other people making PRs to the .NET area of the content wouldn't occur often.

we'll want to set up a semconv-dotnet-approvers team (similar to https://github.com/orgs/open-telemetry/teams/semconv-jvm-approvers) and update CODEOWNERS, similar to

# JVM semantic conventions approvers
/model/metrics/jvm-* @open-telemetry/specs-semconv-approvers @open-telemetry/semconv-jvm-approvers
/docs/jvm/ @open-telemetry/specs-semconv-approvers @open-telemetry/semconv-jvm-approvers

@JamesNK
Copy link
Contributor

JamesNK commented Sep 13, 2023

.NET 8 RC1 shipped with these counter names. What is remaining to do before this can be merged?

@lmolkova
Copy link
Contributor Author

we'll want to set up a semconv-dotnet-approvers team

@trask sounds great! is there a process to request a new group or we do it ad-hoc?

@reyang
Copy link
Member

reyang commented Sep 27, 2023

is there a process to request a new group or we do it ad-hoc?

https://github.com/open-telemetry/semantic-conventions/issues?q=is%3Aissue++request+approvers

Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

I'm wondering about the plan for metrics included in this PR that aren't really .NET specific (dns.* and http.client.*).

Are we planning to pull those out eventually into domain specific conventions (HTTP, and the not yet existing DNS)? If so, are .NET folks willing to accommodate any possible changes resulting from that? If not, aren't we running the risk of introducing possibly conflicting conventions for the same thing (e. g. open HTTP client connections as reported by .NET, and as defined in domain-specific HTTP conventions)?

docs/dotnet/dotnet-http-metrics.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-aspnetcore-metrics.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-http-metrics.md Outdated Show resolved Hide resolved
@lmolkova
Copy link
Contributor Author

Are we planning to pull those out eventually into domain specific conventions (HTTP, and the not yet existing DNS)? If so, are .NET folks willing to accommodate any possible changes resulting from that? If not, aren't we running the risk of introducing possibly conflicting conventions for the same thing (e. g. open HTTP client connections as reported by .NET, and as defined in domain-specific HTTP conventions)?

@pyohannes

I will leave it to .NET folks to decide/confirm, but my considerations are:

  1. The unification process would likely bring breaking changes (names, attributes, etc) if it happens
  2. If a new standard+stable metric for something like DNS is introduced, .NET will have to adjust or stay incompatible.
  3. I assume breaking changes would be considered one-by-one and if the decision is to break, it should be possible with the next major.NET version (which is now happening yearly)

I'm happy to extract common parts and share them as experimental semcov. If anything significant changes there p3 (above) would still apply

Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

Thanks @lmolkova, my main concerns have been addressed.

I will leave it to .NET folks to decide/confirm, but my considerations are:

  1. The unification process would likely bring breaking changes (names, attributes, etc) if it happens
  2. If a new standard+stable metric for something like DNS is introduced, .NET will have to adjust or stay incompatible.
  3. I assume breaking changes would be considered one-by-one and if the decision is to break, it should be possible with the next major.NET version (which is now happening yearly)

Fair. I still find it problematic to have .NET specific semantic conventions which define metrics that aren't .NET specific (HTTP and DNS), I'd rather have those defined in the domain specific sections and .NET use them from there.

I think this can pose problems if those .NET specific semantic conventions are declared stable: if folks want to add language-independent DNS metrics after .NET specific semantic are declared stable, one is either constrained to take over what .NET has defined without breaking it, or one ends up with conflicting definitions for DNS metrics.

However, this isn't a grave problem as long as .NET semantic conventions aren't declared stable, so I'm fine approving this as initial experimental state.

docs/dotnet/dotnet-dns-metrics.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-dns-metrics.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-dns-metrics.md Show resolved Hide resolved
docs/dotnet/dotnet-http-metrics.md Outdated Show resolved Hide resolved
Copy link

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

Client part LGTM. Thanks for the patience with the reviews!

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

I'm good with this hitting the Semconv repository for documentation purposes.

However, I would like to see the dotnet semconv approvers group created before merge.

.github/CODEOWNERS Outdated Show resolved Hide resolved
docs/dotnet/dotnet-kestrel-metrics.md Show resolved Hide resolved
@lmolkova
Copy link
Contributor Author

I'm good with this hitting the Semconv repository for documentation purposes.
However, I would like to see the dotnet semconv approvers group created before merge.

thank you @jsuereth ! The group already exists - #355.
So I think we're ready to merge (in case you're ready to resolve the last conversation #283 (comment))

.github/CODEOWNERS Outdated Show resolved Hide resolved
@jsuereth jsuereth merged commit fe0a3d7 into open-telemetry:main Dec 11, 2023
9 checks passed
pyohannes pushed a commit to pyohannes/semantic-conventions that referenced this pull request Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.