-
Notifications
You must be signed in to change notification settings - Fork 299
Add 'how to define spans' meta-guidance #3240
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
base: main
Are you sure you want to change the base?
Conversation
| Define spans when: | ||
|
|
||
| - The corresponding operation is important for observability. | ||
| - The operation has duration. | ||
| - A new trace context should be created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should all or any of these conditions be fulfilled? Are any points optional?
Maybe consider similar phrasing to the Defining attributes conditions.
| Define spans when: | |
| - The corresponding operation is important for observability. | |
| - The operation has duration. | |
| - A new trace context should be created. | |
| Define spans when all of the following apply: | |
| - The corresponding operation is important for observability. | |
| - The operation has duration. | |
| - A new trace context should be created. |
| Don't try to capture all available properties as span attributes. Telemetry is | ||
| lossy, and it's important to assess the value-to-cost ratio of each referenced attribute. | ||
|
|
||
| Capture the details of that specific operation. Parent operations or sub-operations | ||
| will have their own spans. | ||
|
|
||
| For example, when recording a call to upload a file to an object store, | ||
| include the endpoint, collection, and object identifier. Don't include details of | ||
| the underlying HTTP/gRPC requests unless there is a strong reason to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to attributes section
| messaging queue, GenAI model, object store collection), input parameters, and | ||
| result properties that should be recorded on the span. | ||
|
|
||
| - When referencing attributes, refine its properties: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - When referencing attributes, refine its properties: | |
| - When referencing an attribute, refine its properties: |
|
|
||
| - The corresponding operation is important for observability. | ||
| - The operation has duration. | ||
| - A new trace context should be created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a link for trace context? maybe call it span context(?)
| - The operation has duration. | ||
| - A new trace context should be created. | ||
|
|
||
| Don't define spans for point-in-time telemetry that does not need new context - use events instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the parts about not needing a new context is going to be confusing for most readers
wdyt of leaving that out of the main section and have an "exception" section below for messaging?
| Don't try to capture all available properties as span attributes. Telemetry is | ||
| lossy, and it's important to assess the value-to-cost ratio of each referenced attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't follow the tie in to being lossy
| > the duration of the same operation type. For example, the `http.client.request.duration` | ||
| > metric is recorded alongside the corresponding HTTP client span. | ||
|
|
||
| A span definition includes the following sections: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't follow this line, is it a heading for the sections below?
| - When the span starts and ends. | ||
| - If this span represents a client call, specify whether it captures the logical call | ||
| (as observed by the API caller) or the physical call (per-try/per-attempt). | ||
| - Define a span per different operation type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this is saying, maybe a short example could help?
|
|
||
| Certain conditions can't be clearly classified as errors or not-errors (cancellations, | ||
| HTTP 404 and many others). Avoid using strict requirements - allow instrumentations | ||
| to leverage context they might have to provide accurate status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| to leverage context they might have to provide accurate status. | |
| to leverage context they might have to provide a more accurate status. |
| All span definitions MUST include a specific span kind. Don't mix definitions of | ||
| different span kinds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't mix definitions of different span kinds.
maybe this could be clearer, something like a span type(?) should only have one span kind?
|
|
||
| - When referencing attributes, refine its properties: | ||
| - Specify if an attribute is relevant for head-sampling. Such attributes should be | ||
| provided at start time and would be passed to the sampler. Usually, these are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| provided at start time and would be passed to the sampler. Usually, these are | |
| provided at start time so that they will be passed to the sampler. Usually, these are |
| - Include some form of operation name that allows identifying different operations | ||
| of the same type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"identifying different operations of the same type" seems vague
Maybe "important / useful categories of operations"?
Or could be even more specific and reframe it around {action} {target}?
|
|
||
| - Static text should not be included, but can be used as a fallback. | ||
|
|
||
| E.g., we use `GET /{controller}/{action}` instead of `HTTP GET /{controller}/{action}`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the {action} here was meant to represent a URL path variable and not the action from the span name pattern.
Fixes #1853