api: update the tracer api #131
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmehala
requested review from
Anilm3 and
pablomartinezbernardo
and removed request for
a team
pablomartinezbernardo
approved these changes
Jun 19, 2024
Contributor
pablomartinezbernardo
left a comment
pablomartinezbernardo
left a comment
There was a problem hiding this comment.
What is our policy for incompatible changes in the sdk? We are not following semver right?
Collaborator
Author
|
I think we used to follow semver. At least, that's the policy we are following rn. |
dgoffredo
reviewed
Jul 9, 2024
Comment on lines
76
to
78
| // new trace (see `create_span`). Optionally specify a `config` indicating | ||
| // the attributes of the span. If a failure occurs, then return an error. | ||
| // Note that the absence of a span to extract is not considered an error. |
Contributor
There was a problem hiding this comment.
// Return a span extracted from the specified `reader` (see `extract_span`).
// If there is no span to extract, or if an error occurs during extraction,
// then return a span that is the root of a new trace (see `create_span`).
// Optionally specify a `config` indicating the attributes of the span.Fight the entropy!
At least, that's what the name of the function suggest and I think the desired behavior
Desired by whom? The advantage of allowing extraction errors to be returned is that the caller can, if desired, log the error. I anticipated "traces/sampling broken in Istio thingy" bugs whose root cause is extraction failing without making any noise. Likely you weighed this possibility and decided the benefit of Expected<Span> → Span outweighed the risk.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Update the API:
extract_or_create_spanalways returns a span, either the extracted one or a new span if the extraction fail.override_sampling_priorityacceptsSamplingPriority.Motivation
create_spanalways returns a span, whileextract_spanmay or may not return a span. Logically, one can think the behavior ofextract_or_create_spanshould first attempt toextract_span, and if the extraction fails, it should then callcreate_span. At least, that's what the name of the function suggest and I think the desired behavior.The other one is for simplicity's sake.