TrueCloudLab/frostfs-observability
21
0
Fork 10
generated from TrueCloudLab/basic
Code Issues 4 Pull requests Projects Releases Packages Wiki Activity Actions

[#15] tracing: Add events for grpc stream calls #18

Merged
dstepanov-yadro merged 1 commit from r.loginov/frostfs-observability:feature/15-add_track_low_level_grpc_stream_calls into master 2024-11-26 09:24:48 +00:00
Conversation 5 Commits 1 Files changed 3 +44 -6
r.loginov commented 2024-11-20 08:36:50 +00:00
Member
Copy link

At the moment, I have added one event per specific action. It looks like this in jaeger:
For the client:
image
For the server:
image

There are a number of points that I would like to highlight/discuss.

  1. What do you think about adding additional events for:
    • End of requests. That is, in total, there will be two events for each action: the beginning of the action and the end of the action.
    • Informing about the status of sending/receiving a message with information about the error, if it is present.
  2. Regarding attributes: at the moment I use two attributes:
    • event.name an attribute indicating the unique name of the event. It must be present according to the recommendations from the opentelemetry specification.
    • message.type is an attribute indicating the type of message. I'm not sure if it's really useful, if you think you don't need it, let me know.
      If you have any ideas what other attributes can be added that will be useful for tracking call tracing, suggestions are welcome.

close #15

At the moment, I have added one event per specific action. It looks like this in jaeger: For the client: ![image](/attachments/9412ceff-5938-48d9-a1a5-397dfa852385) For the server: ![image](/attachments/c4a14715-49ad-4739-aa28-1864b2bfa2fa) There are a number of points that I would like to highlight/discuss. 1. What do you think about adding additional events for: - End of requests. That is, in total, there will be two events for each action: the beginning of the action and the end of the action. - Informing about the status of sending/receiving a message with information about the error, if it is present. 2. Regarding attributes: at the moment I use two attributes: - `event.name` an attribute indicating the unique name of the event. It must be present according to the recommendations from the [opentelemetry specification](https://opentelemetry.io/docs/specs/semconv/general/events/#general-event-semantics). - `message.type` is an attribute indicating the type of message. I'm not sure if it's really useful, if you think you don't need it, let me know. If you have any ideas what other attributes can be added that will be useful for tracking call tracing, suggestions are welcome. close #15
image.png
80 KiB
image.png
85 KiB
r.loginov self-assigned this 2024-11-20 08:36:50 +00:00
r.loginov added 1 commit 2024-11-20 08:36:50 +00:00
[#15] tracing: Add events for grpc stream calls
All checks were successful
DCO action / DCO (pull_request) Successful in 1m21s
Details
Tests and linters / Staticcheck (pull_request) Successful in 1m25s
Details
Tests and linters / Tests (pull_request) Successful in 1m40s
Details
Tests and linters / Tests with -race (pull_request) Successful in 1m53s
Details
Tests and linters / Lint (pull_request) Successful in 2m22s
Details
f1ed8fd502 
Signed-off-by: Roman Loginov <r.loginov@yadro.com>
fyrchik requested changes 2024-11-20 08:46:40 +00:00
Dismissed
tracing/grpc/client.go Outdated
@ -39,6 +44,9 @@ func (cs *clientStream) Trailer() metadata.MD {
}
func (cs *clientStream) CloseSend() error {
cs.span.AddEvent("closing the send direction of the stream", trace.WithAttributes(
fyrchik commented 2024-11-20 08:46:21 +00:00
Owner
Copy link

Please, take a look at https://opentelemetry.io/docs/specs/semconv/general/events/ and https://opentelemetry.io/docs/specs/semconv/general/attribute-naming/

I believe what you use as a value for a event.name attribute, should be the first argument to AddEvent.
Event names in this PR don't follow semantic conventions.

Please, take a look at https://opentelemetry.io/docs/specs/semconv/general/events/ and https://opentelemetry.io/docs/specs/semconv/general/attribute-naming/ I believe what you use as a value for a `event.name` attribute, should be the first argument to `AddEvent`. Event names in this PR don't follow semantic conventions.
fyrchik commented 2024-11-20 08:56:05 +00:00
Owner
Copy link

Sorry, it seems you are right, opentelemetry itself uses human readable names in the first argument.

Sorry, it seems you are right, opentelemetry itself uses human readable names in the first argument.
fyrchik marked this conversation as resolved
r.loginov requested review from fyrchik 2024-11-20 08:46:44 +00:00
r.loginov requested review from acid-ant 2024-11-20 08:46:45 +00:00
r.loginov requested review from dstepanov-yadro 2024-11-20 08:46:46 +00:00
r.loginov requested review from alexvanin 2024-11-20 08:46:47 +00:00
dstepanov-yadro commented 2024-11-20 09:13:53 +00:00
Member
Copy link

First of all, thanks for the PR.
Аttachments are not opening. Can you attach images of what the events look like in jaeger?
There is also an implementation proposal: add an event not only at the beginning of receiving or sending, but also at the end.

First of all, thanks for the PR. Аttachments are not opening. Can you attach images of what the events look like in jaeger? There is also an implementation proposal: add an event not only at the beginning of receiving or sending, but also at the end.
👍 1
r.loginov commented 2024-11-20 09:54:22 +00:00
Author
Member
Copy link

Аttachments are not opening. Can you attach images of what the events look like in jaeger?

Screenshots from jaeger have been updated, they should be displayed now.

> Аttachments are not opening. Can you attach images of what the events look like in jaeger? Screenshots from jaeger have been updated, they should be displayed now.
r.loginov force-pushed feature/15-add_track_low_level_grpc_stream_calls from f1ed8fd502 to 07faf91c46 2024-11-20 12:30:22 +00:00 Compare
r.loginov commented 2024-11-20 12:31:49 +00:00
Author
Member
Copy link

Events have been added to the end of calls. In jaeger it looks like this:
Client:
image
Server:
image

Events have been added to the end of calls. In jaeger it looks like this: Client: ![image](/attachments/e0aaf719-eb0a-4f33-a313-8f483921a5f2) Server: ![image](/attachments/e7dbbe20-05ca-4df2-b570-e716e2ed3704)
image.png
152 KiB
image.png
120 KiB
fyrchik commented 2024-11-20 12:37:21 +00:00
Owner
Copy link

There is some inconsistency between wording: in start ..ing, start is a verb, and in end of ..., end is a noun.
I suggest using message receive start/end or start/finish receiving a message.

There is some inconsistency between wording: in `start ..ing`, `start` is a verb, and in `end of ...`, `end` is a noun. I suggest using `message receive start/end` or `start/finish receiving a message`.
👍 1
r.loginov force-pushed feature/15-add_track_low_level_grpc_stream_calls from 07faf91c46 to c8283d96a3 2024-11-20 12:53:07 +00:00 Compare
alexvanin reviewed 2024-11-21 11:17:44 +00:00
tracing/grpc/client.go Outdated
@ -40,2 +45,4 @@
func (cs *clientStream) CloseSend() error {
cs.span.AddEvent("start closing the send direction of the stream", trace.WithAttributes(
attribute.String("event.name", "client.stream.close.send.start")),
alexvanin commented 2024-11-21 11:17:44 +00:00
Owner
Copy link

Is there any difference between this code and

cs.span.AddEvent("client.stream.close.send.start")

I mean, event name and description are basically self explanatory. Maybe we can save couple of bytes during tracing transmission to collector?

What you think @fyrchik, @dstepanov-yadro ?

Is there any difference between this code and ``` cs.span.AddEvent("client.stream.close.send.start") ``` I mean, event name and description are basically self explanatory. Maybe we can save couple of bytes during tracing transmission to collector? What you think @fyrchik, @dstepanov-yadro ?
fyrchik commented 2024-11-21 11:38:08 +00:00
Owner
Copy link

I have looked into opentelemetry code and they use names which do NOT conform to the semantic convention described in docs b99d2b8178/bridge/opencensus/internal/span.go (L84)
So I am not sure how to write AddEvent right.

But I like your suggestion a lot more, it is smaller and seems to achieve exactly what we need.

I have looked into opentelemetry code and they use names which do NOT conform to the semantic convention described in docs https://github.com/open-telemetry/opentelemetry-go/blob/b99d2b81783dd3d27201393fc0e741a6fb4a8d6b/bridge/opencensus/internal/span.go#L84 So I am not sure how to write `AddEvent` right. But I like your suggestion a lot more, it is smaller and seems to achieve exactly what we need.
dstepanov-yadro commented 2024-11-22 10:46:22 +00:00
Member
Copy link

I agree with @fyrchik

I agree with @fyrchik
r.loginov force-pushed feature/15-add_track_low_level_grpc_stream_calls from c8283d96a3 to 37bd758211 2024-11-25 14:03:46 +00:00 Compare
r.loginov commented 2024-11-25 14:06:42 +00:00
Author
Member
Copy link

At the moment, jaeger looks like this:
Client:
image
Server:
image

At the moment, jaeger looks like this: Client: ![image](/attachments/90e1a501-7fea-4bfa-9259-fedee4a2ed02) Server: ![image](/attachments/4bc3542c-e702-46a8-ab48-a875fc8689d3)
image.png
139 KiB
image.png
145 KiB
fyrchik approved these changes 2024-11-26 08:06:19 +00:00
dkirillov approved these changes 2024-11-26 09:07:41 +00:00
dstepanov-yadro approved these changes 2024-11-26 09:24:24 +00:00
dstepanov-yadro merged commit 37bd758211 into master 2024-11-26 09:24:48 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
alexvanin
fyrchik
dkirillov
dstepanov-yadro
Labels
Clear labels
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
r.loginov
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-observability#18
No description provided.
Delete branch "r.loginov/frostfs-observability:feature/15-add_track_low_level_grpc_stream_calls"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?