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

Link to proposal for gRPC metrics #627

Merged
merged 15 commits into from
Mar 26, 2024
Merged

Conversation

yashykt
Copy link
Contributor

@yashykt yashykt commented Jan 4, 2024

Changes

Link to specification for gRPC metrics - https://github.com/grpc/proposal/blob/master/A66-otel-stats.md

Merge requirement checklist

On running make markdown-link-check, I got the following errors, but I'm ignoring them since I'm not modifying these files -

  ERROR: 2 dead links found in ./docs/runtime/README.md !
  [✖] https://github.com/open-telemetry/opentelemetry-specification/tree/v1.26.0/specification/document-status.md → Status: 0
  [✖] https://github.com/open-telemetry/oteps/pull/108/files → Status: 0

docs/rpc/grpc.md Outdated Show resolved Hide resolved
docs/rpc/rpc-metrics.md Outdated Show resolved Hide resolved
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 26, 2024
@yashykt yashykt marked this pull request as ready for review January 30, 2024 01:47
@yashykt yashykt requested review from a team January 30, 2024 01:47
@jsuereth
Copy link
Contributor

jsuereth commented Feb 5, 2024

I think there's a few meta-conversations happening over this PR.

  1. Is this PR a breaking change?
  2. Can gRPC link out to its new o11y specification from the OTEL semconv?
  3. Should gRPC participate in generic OpenTelemetry RPC conventions?

Let's try to tone down this PR to just addressing one thing, and we can follow up on the rest. My comments on each.

Is this PR a breaking change?

As written, yes. I would separate the PR that attempts to remove gRPC from generic RPC metrics from the one which adds links to gRPC's own o11y specification.

Can gRPC link out to its new o11y specification form the OTEL semconv?

We allow .NET, e.g. to define their client-library semconv here for OpenTelemetry users. I think having gRPC define its own metrics is in line with that. We can/should accept a link from our semconv on gRPC to the gRPC-owned o11y specification. Generally, OpenTelemetry is more than happy when projects take their own self-o11y into hand and own it / drive it forward themselves.

Should gRPC participate in generic OpenTelemetry RPC conventions?

This is not something OpenTelemetry can force. I think @markdroth presents a lot of rationale and reasons why gRPC thinks it will be more user-friendly for RPC-implementation-specific semantic conventions. I think folks in our community disagree.

My own opinion is nuanced. I don't see the same kind of general specificaiton/abstraction that keeps databases more tightly coupled (e.g. SQL). However, specifically for gRPC spans, I think providing consistent rpc labels could work.

I suggest for the purpose of this PR we leave this discussion out of it, and have that as a sidebar w/ gRPC team.

My suggestion

I think is true today:

  • gRPC have their own o11y specification focused on metrics that is inline w/ other implementation-specific documentation we've accepted (.NET e.g.).
  • This PR, as it stands, is a breaking change. We cannot allow that without some transition plan.

Let's focus this PR on non-breaking changes to point at gRPC's o11y specification (which we should encourage). @yashykt Could you split the breaking changes into a separate PR?

Let's follow up discussions between our community and @yashykt @markdroth et. al. on gRPC's participation in generic RPC conventions.

@trask
Copy link
Member

trask commented Feb 5, 2024

We allow .NET, e.g. to define their client-library semconv here for OpenTelemetry users. I think having gRPC define its own metrics is in line with that. We can/should accept a link from our semconv on gRPC to the gRPC-owned o11y specification.

👍

We worked heavily with the .NET team when stabilizing the OTel HTTP semantic conventions. They helped influence our HTTP semantic conventions, and we helped influence their .NET specific semantic conventions, and we ended up with a nicely aligned story across both.

I would ❤️ to see the gRPC and OTel communities work together, I believe we can have similar success in the RPC semantic conventions (even more so since we are both under the CNCF umbrella! 🏖️).

@jsuereth and I are both excited to kick off an RPC semantic convention stability working group this year. If there's a time constraint on the gRPC side to get this done, let us know and we can look at kicking it off sooner than later.

@github-actions github-actions bot removed the Stale label Feb 6, 2024
@yashykt
Copy link
Contributor Author

yashykt commented Feb 20, 2024

As suggested, I've removed the breaking changes parts of this PR. I don't think there's a time constraint but I think we should try to resolve the concerns earlier rather than later.

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 13, 2024
@yashykt
Copy link
Contributor Author

yashykt commented Mar 20, 2024

ping

@github-actions github-actions bot removed the Stale label Mar 21, 2024
@lmolkova
Copy link
Contributor

@yashykt could you please add a changelog entry?

@yashykt
Copy link
Contributor Author

yashykt commented Mar 22, 2024

@lmolkova Done

@joaopgrassi joaopgrassi merged commit 4368358 into open-telemetry:main Mar 26, 2024
11 checks passed
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.

8 participants