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.

Sign up for GitHub

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

Jump to bottom

SDK: Add grpc plugin #72

Closed
rghetia opened this issue Aug 2, 2019 · 10 comments · Fixed by #621
Closed

SDK: Add grpc plugin #72

rghetia opened this issue Aug 2, 2019 · 10 comments · Fixed by #621
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package
Milestone
untracked

Comments

@rghetia
Copy link
Contributor

rghetia commented Aug 2, 2019 •
edited
Loading

Create grpc plugin for opentelemetry, similar to ocgrpc for opencensus
@rghetia rghetia added the pkg:SDK Related to an SDK package label Aug 2, 2019
@jmacd jmacd added the area:trace Part of OpenTelemetry tracing label Sep 19, 2019
@jmacd jmacd added this to the Tracing SDK milestone Sep 19, 2019
@iredelmeier
Copy link
Member

iredelmeier commented Sep 26, 2019 •
edited
Loading

(for posterity)

There are (at least) three routes we can go with this:

  1. Use stats.Handler. This is what ocgrpc does.
  2. Use interceptors (UnaryClientInterceptor, StreamClientInterceptor, UnaryServerInterceptor, and StreamServerInterceptor). This is what both opentracing go-grpc and otgrpc do.
  3. Use some combination of both

stats.Handler exposes details such as the uncompressed and wire length of the payload, which aren't accessible through the interceptors.

On the other hand, the stream interceptors enable us to observe individual messages within the stream, rather than treating the whole stream as a single payload. stats.Handler doesn't facilitate this granularity. (Note that neither opentracing package currently does anything per-message, although it would probably be quite useful for many streaming use cases.)

Thoughts?

ETA: As far as consistency across languages goes, I don't believe that any other languages support an equivalent to stats.Handler. Some, but not all, support interceptors.

@rghetia
Copy link
Contributor Author

rghetia commented Sep 26, 2019

Is it possible to associate different traces for each payload in case of stream interceptors? Otherwise long lived stream could end up having one parent creating 100s or 1000s of child span.

Can we support both with an option flag?

@rghetia rghetia modified the milestones: Tracing SDK, Alpha Sep 26, 2019
@iredelmeier
Copy link
Member

Decision on the above, based on today's SIG, is to with interceptors at least for now. Longer term, we may do a combination of both.

@rghetia rghetia removed this from the Alpha v0.2 milestone Oct 3, 2019
@iredelmeier iredelmeier removed their assignment Nov 26, 2019
@iredelmeier
Copy link
Member

iredelmeier commented Nov 26, 2019 •
edited
Loading

Closing this for now as I believe it makes sense to start with the converted Datadog gRPC plugin (as discussed in last week's SIG meeting), which we should be able to make public soon.

@inigohu
Copy link

inigohu commented Apr 1, 2020

Any updates about otgrpc plugin?

@glerchundi
Copy link

Why was this issue closed? I think the decision to go through interceptors is a implementation detail but the feature request to create an interceptor/plugin still applies, right?

@rghetia
Copy link
Contributor Author

rghetia commented Apr 3, 2020

reopening this as requested above.

@rghetia rghetia reopened this Apr 3, 2020
@reicheltp
Copy link
Contributor

I am currently implementing those interceptors for our company. I'll send a PR as soon as they're ready.

@reicheltp reicheltp mentioned this issue Apr 6, 2020
Merged
jmacd pushed a commit that referenced this issue Apr 23, 2020
@reicheltp
Resolve #72 - gRPC Interceptor (#621)
6de3dab 
* Move interceptor to plugin

* Add basic net.peer info

* Ensure that grpc status match span status

See: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-rpc.md#status

* Set rpc.service attribute

* Add StreamClientInterceptor and StreamServerInterceptor

* Fix: golint errors

* Apply automated go.mod changes from make

* Implement suggestions to improve readability
@sudeep-ib
Copy link

The attached PR #621, addresses tracing but does not implement stats. Is there a roadmap to including the stats plugin in opentelemetry. Is there an Issue that tracks it, or we could re-open this one?
@rghetia @jmacd

@rghetia
Copy link
Contributor Author

rghetia commented May 5, 2020

@sudeep-ib please open a new issue for stats.

hstan referenced this issue in hstan/opentelemetry-go Oct 15, 2020
@dependabot @evantorrie @MrAlias
Bump google.golang.org/grpc from 1.28.1 to 1.29.1 (#72)
d923ef6 
* Bump google.golang.org/grpc from 1.28.1 to 1.29.1

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.28.1 to 1.29.1.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.28.1...v1.29.1)

Signed-off-by: dependabot[bot] <[email protected]>

* Push grpc-1.29.1 across all modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: ET <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package
Projects
None yet
Milestone
untracked
Development

Successfully merging a pull request may close this issue.

Resolve #72 - gRPC Interceptor regiocom/opentelemetry-go
8 participants
@glerchundi @iredelmeier @jmacd @reicheltp @pellared @inigohu @rghetia @sudeep-ib