This repository was archived by the owner on Dec 6, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support for more expressive span status API #123
Add support for more expressive span status API #123
Changes from 1 commit
cd147a9
32bab4b
ddb7f49
6c0a4de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How would a named error with a separate message be captured? I would treat these separately and not combine them in one free-form string. This would allow for searching, filtering and grouping by the error name alone but still preserve the message. A backend could, for example, display the number of all
EntityNotFound
errors in one place and filter for them in another place where users can inspect the messages bearing the entities that were not found.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.
The current proposal wouldn't be able to capture this, but we could change the proposal to allow it. I've added some text to the trade-offs and mitigations section mentioning the idea. I'd be fine adding it, not sure if we'll see additional opinions about it?
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.
Personally, I'd add it right from the start. I'd expect users want to capture the information in a way that most closely resembles how it's reported by their respective language or framework, rather than having to combine both in one string that potentially has to be parsed/decomposed for analysis later again.
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.
My original intent wasn't that users would combine strings, but that they would provide either an enumerated status code OR a message, not both. If we anticipate it is common to provide both I agree I'd rather provide an API that easily facilitates that vs. an API that encourages users to create private conventions to merge and split text.
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 expect it to be common enough to support it in the API. Often it will indeed be a type of error that can be enumerated but will not necessarily have an integer representation like we have in HTTP but rather a name of the error (like "Entity not found" or an exception name) with a (detailed) description (e.g. "The entity with ID '7583' was not found in the table 'products'" or an exception message).
Therefore I'm suggesting to have two strings called, for example, name, brief or kind and a separate string called details, message or description.
Happy to hear others' opinions on that.
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.
Capturing an exception with traceback and storing it until export is potentially extremely memory heavy (e.g. in Python the exception has a reference to a traceback, and the traceback has references to all local variables on the stack, preventing them from being GCd).
So the solution to this would probably be to call into some exporter-specific code already when calling this API.
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.
Thanks! I added a comment about this issue and potential solution to the internal details section.
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.
Multiple exceptions can occur during the lifetime of a span and experiencing an exception may or may not result in a non-ok status depending on the circumstances. How can multiple exceptions can handled using this API?
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.
Multiple functions returning error codes may be called during the lifetime of a span and an error return may result in a non-ok status or not.
I think it's up to the user/instrumentation to decide which exception was the semantically important one. Most likely the one that went uncaugth.
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.
Agreed with @Oberon00. I also attempted to capture this concept in the scoping section above but perhaps the wording wasn't good?
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.
Markdown syntax problem:
(you see now why line breaks would be good? 😉 )
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 love reading paragraphs hunting for the changed space character! ... agreed and fixed 👍 )
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.
Note that you can provide a nicely typed API and still use semantic conventions. E.g. the API could just "serialize" whatever you give it into SetAttribute / CreateEvent calls. The difference is that the API between the exporter and the API is then "weakly typed".
To point at another project, the idea of "typed spans", i.e. utilities to have strongly typed APIs for semantic conventions, is at least as old as OpenTelemetry. E.g. see "Typed Spans" in the PR description of open-telemetry/opentelemetry-specification#571
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.
As an additional advantage, the exception/error APIs could be provided as a package separate from the core API, (you could provide a function
com.opentelemetry.contrib.errorutils.ErrorUtils.captureError(Span, other params)
that sets any attributes/events on the span).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.
Thanks! I've added this suggestion to the list of alternatives.
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 see this as completely orthogonal. If we use semantic conventions, they could prescribe span attributes or event attributes or both.
If we have additional API functions, they could be defined on the span or some event (builder) object, and even if defined on the span they could create event-level or span-level data.
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 we are in agreement? Perhaps my second paragraph made it sound like events aren't semantic conventions so I've clarified in the title that events are also conventions.
I've been limiting the scope of the discussion to the tracing API so here I was attempting to distinguish whether the user provides the information via strongly typed API or provides it via loosely typed conventions. I agree that a strongly typed API could store the data internally as attributes/events and doing so probably (but not necessarily) has some implications on the SDK API for reading the data back.
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 added some text in the implementation section of the coming revision to explicitly call out the ability to store the data into attributes or events regardless of the tracing API that provides the data to us.