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

Avoid using gRPC when generic RPC systems have same properties #1914

Merged
merged 5 commits into from
Sep 28, 2021

Conversation

bogdandrutu
Copy link
Member

The concept of "streaming" is supported by multiple protocols including gRPC, but also others.

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

specification/trace/semantic_conventions/rpc.md Outdated Show resolved Hide resolved
semantic_conventions/trace/rpc.yaml Show resolved Hide resolved
@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Sep 9, 2021
The concept of "streaming" is supported by multiple protocols including gRPC, but also others.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@jsuereth
Copy link
Contributor

jsuereth commented Sep 9, 2021

Think this may need a telemetry schema update cc @tigrannajaryan

@bogdandrutu
Copy link
Member Author

@jsuereth no idea since the JSON was added after the last release, only change is that the event applies to all RPC systems :D which I have no idea how to define in schema url change :D

@tigrannajaryan
Copy link
Member

Think this may need a telemetry schema update cc @tigrannajaryan

I do no think this needs a schema update. If I understand correctly this change says that a source that previously was not emitting the event should now do so (i.e. all RPCs, not just gRPC).

There are no changes in the attribute names or values so the schema does not change in any way.

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@bogdandrutu
Copy link
Member Author

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers @open-telemetry/technical-committee all comments are addressed, if no more comments in the next 24h we will merge this.

@bogdandrutu bogdandrutu enabled auto-merge (squash) September 16, 2021 10:37
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Actually, I have one more question about this: The message.id attribute has a very specific definition. Does that make sense for RPC generally? Did it even make sense for gRPC? Should we (temporarily) mark it as deprecated until we decide if it makes sense?

@jsuereth
Copy link
Contributor

@Oberon00 The message.id thing is for streaming RPCs and is the current status quo for gRPC instrumentation See: https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/gRPC.md#message-events

@Oberon00
Copy link
Member

Oberon00 commented Sep 17, 2021

Then maybe that property should be kept as rpc.grpc.message.id?

@jsuereth
Copy link
Contributor

I'd be ok with keeping things grpc specific when we're not sure if it applies to non gRPC systems.

@github-actions
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 Sep 25, 2021
@jsuereth jsuereth removed the Stale label Sep 27, 2021
@carlosalberto
Copy link
Contributor

I'd be ok with keeping things grpc specific when we're not sure if it applies to non gRPC systems.

@bogdandrutu ^ ?

@bogdandrutu
Copy link
Member Author

bogdandrutu commented Sep 28, 2021

@Oberon00 @jsuereth without the ID you cannot map sent with received messages in a streaming RPC, which is one of the critical property to be able to determine network delays, etc. I think it should be a very generic property for any streaming implementation not just gRPC.

@bogdandrutu bogdandrutu merged commit 2031369 into open-telemetry:main Sep 28, 2021
@bogdandrutu bogdandrutu deleted the grpcmessage branch September 29, 2021 11:22
@Oberon00
Copy link
Member

Oberon00 commented Oct 1, 2021

without the ID you cannot map sent with received messages

Well we need some ID, but maybe not one generated in exactly that way. Some RPC systems may already have an ID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants