[chore] Increase emphasis of sampling attributes#2678
[chore] Increase emphasis of sampling attributes#2678thompson-tomo wants to merge 7 commits intoopen-telemetry:mainfrom
Conversation
|
Thanks for spoting that the wrong issue was referenced. In relation to adding attributes before/after span is started, my understanding is there would be no difference in functionality as I am not aware of the start action triggering any functionality. But not 100% sure. |
|
At https://opentelemetry.io/docs/specs/otel/trace/api/, I don't see any requirement for OpenTelemetry API to provide a way to create a Span without starting it immediately. Because of that, I think the semantic conventions don't need to give any recommendations about adding attributes before vs. after starting a span. In .NET though, the Activity API supports the distinction between creating a span and starting it. One can set an ActivityListener.ActivityStarted callback, which can read the attributes of the span and export them to real-time monitoring. (Not over OTLP, which only deals with finished spans.) |
|
This is not a new recommendation, and in fact the wording was simply moved along with the attributes it applies to. Also the schema when writing sem conv, allows for attributes to be indicated that they should be provided at creation time to support head sampling based on attributes. Note the creating of a span with attributes and starting it via the 1 call should be the seen as creating it, adding tags & then starting. |
|
Yes I saw you did not modify YAML files in this pull request. I just meant there is no need to additionally replace the two levels (attributes at span creation for sampling / attributes added later) with three levels (at span creation / before starting / after starting). |
|
So in generally I'm a fan of calling out sampling relevant in a far better fashion. I think highlighting this makes a lot of sense. What I'm not certain of is the incarnation here. This trades off sampling-relevant being highlighted at the expense of not being able to see all attributes you'd expect on a span in one table. Maybe, instead, we should render "Sampling Relevant" in the attribute table when we render Span semantic conventions? |
cc3eb18 to
db44b32
Compare
|
Yes I did think about that however if we merge the tables, we lose the descriptive paragraph which provide context about what it means. If we merge the tables what could the heading be? The only option I can think of is "Value Captured" with options of "Span Creation" & "Any time". |
eda01c7 to
33ce9a2
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
33ce9a2 to
efd6d28
Compare
efd6d28 to
5868caf
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@joaopgrassi i still see the benefit in the capture column scope as it would enable richer, more complete docs. With that being said to progress this PR I have renamed the column to "role" with a value of "head sampling" when appropiate. This ensures that each column retains a clear single purpose. |
|
I'm not convinced we need a new column. I'd still in the opinion that a simple badge as mentioned before is easier/simpler. I don't think we today have a problem where "sampling relevant" is lacking explanation or people are confused. We never had problems or people coming to complain. So, I believe until we have that, this suggestions seems the best compromise: #2678 (comment) |
e56fc86 to
efd840c
Compare
|
I don't see this discussion being about seperate columns being any different to #2970 where it was decided to go seperate. I have tweaked the jinja so only spans gain the additional column. |
efd840c to
fd57b1c
Compare
|
Closing as there does not appear to be interest from the necessary folks in order to move this forward at this time. |

Fixes #2677
Changes
All changes have been contained the jinja templates with an explaination of the changes below:
A good example of the changes is comparing current version to new version
Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.
Merge requirement checklist
[chore]