A72: OpenTelemetry Tracing #389

YifeiZhuang · 2023-08-22T00:16:36Z

Design doc: go/grpc-open-telemetry-tracing-design

yashykt · 2023-09-13T19:37:19Z

I don't see the span name mentioned here. I was going to ask whether we can be consistent on the naming. @dfawley had suggested we "rcvd" instead of "recv" as the shortform for received in the OTel metrics #380

YifeiZhuang · 2023-09-13T21:28:07Z

I don't see the span name mentioned here. I was going to ask whether we can be consistent on the naming. @dfawley had suggested we "rcvd" instead of "recv" as the shortform for received in the OTel metrics #380

The span name will inherit what census uses, like in server Recv.<service name>.<method name>, client side Sent.<service name>.<method name> (A45), as mentioned in New Function in OpenTelemetry Plugin

In the new function we will produce the same tracing information as we produce for Census

But it looks A45 does not mention Recv at all.
rcvd seems fine. When there is decision there, we can certainly make the name consistent here.

A72-open-telemetry-tracing.md

ejona's language suggestion Co-authored-by: Eric Anderson <ejona@google.com>

…into ot-tracing

1. [done] Add that OpenTelemetry module should document that things/configurations will change as OT changes. 2. [done] Move C++ fast path to rationale. Capture two alternatives about the fast path that may have in the future. 3. [done] Update C++ API about adding a new method in OpenTelemetryPluginBuilder, and reference to the current code and the gRFC. 4. [todo] complete the C++ GrpcTraceBinPropagator and GrpcTextMapCarrier

A72-open-telemetry-tracing.md

Fill up C++ sections

A72-open-telemetry-tracing.md

markdroth

Just a few details left here!

PLMK if you have any questions. Thanks!

A72-open-telemetry-tracing.md

yashykt

I think my comments are mostly minor assuming we can easily change the formatting of the event messages and attributes

A72-open-telemetry-tracing.md

yashykt · 2024-04-05T02:43:36Z

A72-open-telemetry-tracing.md

+  events for compressed/uncompressed message sizes, the same below) 
+  with name "Outbound message sent" and the following attributes:
+  * key `message.event.type` with string value "SENT".
+  * key `message.message.id` with integer value of the seq no. The seq no. indicates


how was the namespacing for "message.message.id" decided?

It was from the Otel java opencensus shim. Languages can be different.
I think whenever possible, we can make the name generated from the shim and the one generated directly from otel be consistent. This is not happening because the naming in our census needs improvements. And I am fine with that. @markdroth @ejona86

(https://github.com/open-telemetry/opentelemetry-java/blob/main/opencensus-shim/src/main/java/io/opentelemetry/opencensusshim/SpanConverter.java#L24-L27)

I don't think we have to copy the shim's behavior.

I proposed new names. Let's move forward and fix/vote in API stabilization.

A72-open-telemetry-tracing.md

…remove to transport and form wire

markdroth

Just one issue remaining here!

PLMK if you have any questions. Thanks!

A72-open-telemetry-tracing.md

…ssed message size inbound

markdroth

Thanks for all the edits! This looks great!

A72-open-telemetry-tracing.md

ejona86

Sending what I have. I think yashykt had some good comments.

A72-open-telemetry-tracing.md

ejona86 · 2024-05-31T20:42:44Z

A72-open-telemetry-tracing.md

+  * key `message.message.id` with integer value of the seq no. The seq no. indicates
+    the order of the sent messages on the attempt (i.e., it starts at 0 and is 
+    incremented by 1 for each message sent), the same below.
+  * key named `message.event.size.uncompressed` if the message needs compression,


"needs compression" is a bit difficult. I assume that is "if this message will be compressed." But I'm not certain all languages can do this (know before the message is compressed whether the message will be compressed).

@yashykt could you help confirm whether C++ is able to tell the message needs compression or not when it is reporting the first Event?

Eric is right. Till we actually go and compress, we won't know whether we will compress it.

Also, it is illegal to have an attribute named message.event.size if there is another attribute message.event.size.uncompressed where message.event.size is the namespace.

Refer https://opentelemetry.io/docs/specs/semconv/general/attribute-naming/

it is illegal to have an attribute named message.event.size if there is another attribute message.event.size.uncompressed where message.event.size is the namespace.

Good point. I have two naming proposals:

Extending from opencensus and use "-" to form the message: e.g. "message-sequence-no", "message-size","message-size-compressed".

Follow ot spec and use ".", then we may end up with "grpc.previous.rpc.attempts", "grpc.transparent.retry", "grpc.message.sequence.no", "grpc.message.size", "grpc.compressed.message.size", or similar.

I'm fine either way. Does anyone have any other naming suggestions?

A72-open-telemetry-tracing.md

ejona86 · 2024-05-31T20:58:19Z

A72-open-telemetry-tracing.md

+  events for compressed/uncompressed message sizes, the same below) 
+  with name "Outbound message sent" and the following attributes:
+  * key `message.event.type` with string value "SENT".
+  * key `message.message.id` with integer value of the seq no. The seq no. indicates


I don't think we have to copy the shim's behavior.

YifeiZhuang · 2024-06-04T14:48:33Z

A72-open-telemetry-tracing.md

+  * key `message.message.id` with integer value of the seq no. The seq no. indicates
+    the order of the sent messages on the attempt (i.e., it starts at 0 and is 
+    incremented by 1 for each message sent), the same below.
+  * key named `message.event.size.uncompressed` if the message needs compression,


@yashykt could you help confirm whether C++ is able to tell the message needs compression or not when it is reporting the first Event?

YifeiZhuang · 2024-06-04T14:48:54Z

A72-open-telemetry-tracing.md

+  * key `message.message.id` with integer value of the seq no. The seq no. indicates
+    the order of the received messages on the attempt (i.e., it starts at 0 and is
+    incremented by 1 for each message received), the same below.
+  * key named `message.event.size.compressed` if the message needs decompression,


@yashykt same here for inbound.

On the inbound message, we know from the wire format whether the message was compressed, so this is fine.

But, it is illegal to have an attribute named message.event.size if there is another attribute message.event.size.uncompressed where message.event.size is the namespace.

…cing

…into ot-tracing

yashykt · 2024-06-05T17:29:37Z

A72-open-telemetry-tracing.md

+* When the application sends an outbound message, add Event(s)
+  (it depends on implementation whether there is a single event or an additional 
+  separate event with name "Outbound message compressed" for compressed message size) 
+  with name "Outbound message sent" and the following attributes:


I think we talked about this earlier, but "sent" in "Outbound message sent" is confusing. Can we drop it?
For C++, this would otherwise mean that we would first see -
"Outbound message sent" and then "Outbound message compressed"

"Outbound message sent" and then "Outbound message compressed"

It looks fine. "sent" means this trace happens somewhere from application to the wire, and we made that vague and general(decision made from a discussion). Are you confused because compression should not happen after "sent"?

yashykt · 2024-06-05T17:37:35Z

A72-open-telemetry-tracing.md

+  * key `message.message.id` with integer value of the seq no. The seq no. indicates
+    the order of the received messages on the attempt (i.e., it starts at 0 and is
+    incremented by 1 for each message received), the same below.
+  * key named `message.event.size.compressed` if the message needs decompression,


On the inbound message, we know from the wire format whether the message was compressed, so this is fine.

But, it is illegal to have an attribute named message.event.size if there is another attribute message.event.size.uncompressed where message.event.size is the namespace.

yashykt · 2024-06-05T17:38:11Z

A72-open-telemetry-tracing.md

+  * key `message.message.id` with integer value of the seq no. The seq no. indicates
+    the order of the sent messages on the attempt (i.e., it starts at 0 and is 
+    incremented by 1 for each message sent), the same below.
+  * key named `message.event.size.uncompressed` if the message needs compression,


Also, it is illegal to have an attribute named message.event.size if there is another attribute message.event.size.uncompressed where message.event.size is the namespace.

Refer https://opentelemetry.io/docs/specs/semconv/general/attribute-naming/

…rst mesage size even when it does need compression

YifeiZhuang · 2024-06-18T22:27:10Z

@yashykt Thanks for the review. Eric and I made improvements on the otel message size reporting details (mainly 8afa317), please take a look and comment whether it is feasible in C++.

cc. @ejona86 @dfawley

A72-open-telemetry-tracing.md

YifeiZhuang added 4 commits August 21, 2023 17:06

start

1d5b48c

rename to A67

67e34e8

Merge remote-tracking branch 'upstream/master' into ot-tracing

392a533

rename A72

7f668a7

YifeiZhuang force-pushed the ot-tracing branch from 26ab562 to 43d09ef Compare August 29, 2023 15:25

minor fixes

0041641

YifeiZhuang force-pushed the ot-tracing branch from 43d09ef to 0041641 Compare August 29, 2023 15:29

YifeiZhuang requested review from ejona86, markdroth, dfawley and fengli79 August 29, 2023 15:31

YifeiZhuang marked this pull request as ready for review August 29, 2023 15:32

yashykt reviewed Sep 18, 2023

View reviewed changes

dragon3 mentioned this pull request Nov 13, 2023

Align gRPC server status code to span status code connectrpc/otelconnect-go#144

Merged

ejona86 reviewed Dec 18, 2023

View reviewed changes

YifeiZhuang and others added 5 commits December 19, 2023 15:05

add email thread discussion, and text changes from ejona

7a6e596

Apply suggestions from code review

6bc027e

ejona's language suggestion Co-authored-by: Eric Anderson <ejona@google.com>

Merge branch 'ot-tracing' of https://github.com/YifeiZhuang/proposal …

5bb3823

…into ot-tracing

minor fix

5dbbc13

zasweq reviewed Jan 9, 2024

View reviewed changes

A72-open-telemetry-tracing.md Outdated Show resolved Hide resolved

yashykt and others added 3 commits January 17, 2024 03:46

Fill up C++ sections

8631cee

Reviewer comments

528dd82

Merge pull request #2 from yashykt/ot-tracing

cf24140

Fill up C++ sections

yashykt reviewed Jan 18, 2024

View reviewed changes

A72-open-telemetry-tracing.md Show resolved Hide resolved

yashykt reviewed Jan 18, 2024

View reviewed changes

A72-open-telemetry-tracing.md Show resolved Hide resolved

trace info, custom binary header validation

517f340

YifeiZhuang force-pushed the ot-tracing branch from 81dd9a4 to 517f340 Compare January 19, 2024 01:34

markdroth reviewed Apr 4, 2024

View reviewed changes

yashykt approved these changes Apr 5, 2024

View reviewed changes

clarify the second message for compressed/uncompressed message name, …

3e26373

…remove to transport and form wire

YifeiZhuang requested a review from markdroth April 5, 2024 22:30

zasweq mentioned this pull request Apr 8, 2024

stats/opentelemetry: Add OpenTelemetry instrumentation component grpc/grpc-go#7066

Closed

markdroth reviewed Apr 9, 2024

View reviewed changes

A72-open-telemetry-tracing.md Outdated Show resolved Hide resolved

scheme for the name of uncompressed message size outbound, and compre…

3885cd8

…ssed message size inbound

markdroth approved these changes Apr 10, 2024

View reviewed changes

YifeiZhuang commented Apr 10, 2024

View reviewed changes

A72-open-telemetry-tracing.md Show resolved Hide resolved

XuanWang-Amos added 2 commits April 10, 2024 21:58

Add Python draft API

c866de5

Fix format issue

a346c24

dfawley approved these changes Apr 22, 2024

View reviewed changes

A72-open-telemetry-tracing.md Outdated Show resolved Hide resolved

A72-open-telemetry-tracing.md Outdated Show resolved Hide resolved

ejona86 mentioned this pull request Apr 23, 2024

visibility into client-side queuing and flow control grpc/grpc-java#11114

Open

ejona86 reviewed May 31, 2024

View reviewed changes

YifeiZhuang commented Jun 4, 2024

View reviewed changes

YifeiZhuang added 3 commits June 4, 2024 09:07

change go/java api, remove mesage type, rename event attributs

66b40d4

Merge branch 'master' of https://github.com/grpc/proposal into ot-tra…

41c2ed2

…cing

Merge branch 'ot-tracing' of https://github.com/YifeiZhuang/proposal …

68443a0

…into ot-tracing

yashykt reviewed Jun 5, 2024

View reviewed changes

yashykt requested changes Jun 5, 2024

View reviewed changes

YifeiZhuang added 4 commits June 5, 2024 12:11

implementation may not know it needs compression whe reporting the fi…

a199618

…rst mesage size even when it does need compression

improve compression message name and add example

8afa317

fix typo

78f3d71

refer to event name, update oc/ot field name mapping, and fix typo

9a219d3

ejona86 approved these changes Jun 19, 2024

View reviewed changes

A72-open-telemetry-tracing.md Outdated Show resolved Hide resolved

move use of propagator before c++ example section

06ec130

yashykt approved these changes Jun 20, 2024

View reviewed changes

YifeiZhuang merged commit 596b629 into grpc:master Jun 20, 2024
1 check passed

YifeiZhuang deleted the ot-tracing branch June 20, 2024 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A72: OpenTelemetry Tracing #389

A72: OpenTelemetry Tracing #389

YifeiZhuang commented Aug 22, 2023 •

edited

yashykt commented Sep 13, 2023

YifeiZhuang commented Sep 13, 2023

markdroth left a comment

yashykt left a comment •

edited

yashykt Apr 5, 2024

YifeiZhuang Apr 5, 2024

ejona86 May 31, 2024

YifeiZhuang Jun 4, 2024

markdroth left a comment

markdroth left a comment

ejona86 left a comment

ejona86 May 31, 2024

YifeiZhuang Jun 4, 2024

yashykt Jun 5, 2024

yashykt Jun 5, 2024

YifeiZhuang Jun 5, 2024

ejona86 May 31, 2024

YifeiZhuang Jun 4, 2024

YifeiZhuang Jun 4, 2024

yashykt Jun 5, 2024

yashykt Jun 5, 2024

YifeiZhuang Jun 5, 2024

yashykt Jun 5, 2024

yashykt Jun 5, 2024

YifeiZhuang commented Jun 18, 2024

A72: OpenTelemetry Tracing #389

A72: OpenTelemetry Tracing #389

Conversation

YifeiZhuang commented Aug 22, 2023 • edited

yashykt commented Sep 13, 2023

YifeiZhuang commented Sep 13, 2023

markdroth left a comment

Choose a reason for hiding this comment

yashykt left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YifeiZhuang commented Jun 18, 2024

YifeiZhuang commented Aug 22, 2023 •

edited

yashykt left a comment •

edited